Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove copy of Niworkflows in favor of dependency #709

Merged
merged 19 commits into from
May 2, 2024

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Mar 4, 2024

Changes proposed in this pull request

  • Remove copy of niworkflows.
  • Add niworkflows as a requirement.
  • Bump svgutils requirement from 0.3.0 to 0.3.4 (for niworkflows compliance).
  • Add copies of interfaces from the qsiprep copy of niworkflows to the interfaces.niworkflows module, for niworkflows interfaces that have been removed (or renamed in such a way that I couldn't track down the new version) from niworkflows in the versions since a copy was made in qsiprep.
    • RobustMNINormalizationRPT
  • Move plot_denoise, plot_acpc, and slices_from_bbox functions to viz.utils module.

@tsalo tsalo marked this pull request as ready for review March 28, 2024 15:18
@tsalo tsalo added the refactor Changes to the codebase that don't impact workflow inputs or outputs. label Mar 28, 2024
@tsalo tsalo changed the title Remove Niworkflows Remove copy of Niworkflows in favor of dependency Mar 28, 2024
@mattcieslak
Copy link
Collaborator

I'm so happy to see that this works. In an unrelated PR, do you know if this change means we can remove the CRN_SHARED_DATA and /niworkflows directory from the docker build?

@tsalo
Copy link
Member Author

tsalo commented Apr 12, 2024

I don't know about CRN_SHARED_DATA, but it does seem like /niworkflows could be removed.

@tsalo
Copy link
Member Author

tsalo commented Apr 22, 2024

@mattcieslak it looks like you made changes to something in the local niworkflows copy. Do you want me to address the conflict, or would you be willing to (just because you would know better what you changed)?

@mattcieslak
Copy link
Collaborator

it's bizarre, I can't click the "resolve conflicts" button because it says the conflicts are too complex to resolve in a web browser. That's a new one

@tsalo
Copy link
Member Author

tsalo commented Apr 22, 2024

I think it's because my branch removes the file completely, while you actually changed the contents in master.

@mattcieslak mattcieslak merged commit 990f2e5 into PennLINC:master May 2, 2024
3 checks passed
@tsalo tsalo deleted the drop-niworkflows branch May 2, 2024 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Changes to the codebase that don't impact workflow inputs or outputs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants