-
Notifications
You must be signed in to change notification settings - Fork 5
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
[ENH] Add derivatives command and pipeline-catalog
submodule
#349
base: main
Are you sure you want to change the base?
Conversation
- participant_id col of existing examples also updated to match Nipoppy
- prevents circular import errors since some utils require models.py, which in turn requires IO utils
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #349 +/- ##
==========================================
+ Coverage 98.08% 98.34% +0.26%
==========================================
Files 13 16 +3
Lines 782 966 +184
==========================================
+ Hits 767 950 +183
- Misses 15 16 +1 ☔ View full report in Codecov by Sentry. |
…el/bagelbids into add-derivatives-command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @alyssadai for this very big PR and for setting up a brand new subcommand!
I really like the extensive testing of the new functionalities.
That said, I have a couple of comments on the code itself that I think make sense to address before merging. Everything runs, so I'll let you take a look at them and tell me when you're ready to merge before approving.
As I mentioned today, I don't know yet how to better break down or review such a big feature, but we should find a more efficient way - this takes a lot of uninterrupted concentration to review well and thus a lot of time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much @alyssadai, looks great.
🧑🍳
Keeping this PR open for now to merge branch for #346 back into this one |
@context
and outputs frombagel derivatives
neurobagel_examples#34@context
)Changes proposed in this pull request:
bagel derivatives
commandses-nb01
ImagingSessionfile_utils.py
to avoid circular importsderivative_utils.py
to hold helper funcs forbagel derivatives
@context
in each command (previously, we were stripping it from an input JSONLD and re-adding it at the end. This led to sometimes ending up with an incomplete@context
without all the terms used/added to the dataset by the command that was just run)Checklist
This section is for the PR reviewer
[ENH]
,[FIX]
,[REF]
,[TST]
,[CI]
,[MNT]
,[INF]
,[MODEL]
,[DOC]
) (see our Contributing Guidelines for more info)skip-release
(to be applied by maintainers only)Closes #XXXX
For new features:
For bug fixes: