-
Notifications
You must be signed in to change notification settings - Fork 2
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
Draft pre-init ingression #102
Conversation
|
None of my versions pinned for |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #102 +/- ##
==========================================
+ Coverage 33.68% 33.74% +0.05%
==========================================
Files 56 56
Lines 6937 6855 -82
Branches 910 901 -9
==========================================
- Hits 2337 2313 -24
+ Misses 4496 4437 -59
- Partials 104 105 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Now that tests are passing, @smeisler do you want to take over this PR and add any tests you might want? |
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.
A few minor questions, but I think this looks great
@@ -59,7 +59,10 @@ HCP Young Adult Preprocessed Data | |||
================================= | |||
|
|||
To use minimally preprocessed dMRI data from HCP-YA specify ``--input-type hcpya``. | |||
The included FNIRT transforms are usable directly. NOTE: this does not work yet. | |||
Note that the transforms to/from MNI space are not able to be used at this time. Please note that if you have the |
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.
great idea to add this to the docs
pyproject.toml
Outdated
"nibabel <= 5.2.0", | ||
"nilearn == 0.10.1", | ||
"nibabel <= 6.0.0", | ||
"nilearn", |
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.
Is there a need to unpin this one?
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.
ah this was a holdout from some dependency differences between qsirecon and i2q, we should be able to pin it
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.
done in last commit
qsirecon/cli/parser.py
Outdated
|
||
# Make fake BIDS files | ||
bids_scaffold = str(load_resource("bids_scaffold")) | ||
if not op.exists(op.join(config.execution.bids_dir, "dataset_description.json")): |
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.
could you switch this to use pathlib instead of op? I think we're going to try to consistently use it to be in line with nipreps
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.
Think I changed this, untested so far:
if config.workflow.input_type in ("hcpya", "ukb"):
import shutil
from ingress2qsirecon.data import load_resource
from ingress2qsirecon.utils.functions import create_layout
from ingress2qsirecon.utils.workflows import create_ingress2qsirecon_wf
# Fake BIDS directory to be created
config.execution.bids_dir = work_dir / "bids"
# Make fake BIDS files
bids_scaffold = load_resource("bids_scaffold")
if not (config.execution.bids_dir / "dataset_description.json").exists():
shutil.copytree(
bids_scaffold,
config.execution.bids_dir,
dirs_exist_ok=True,
)
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.
Excellent!
Just a template for integrating @smeisler's ingression package.
Changes proposed in this pull request
Documentation that should be reviewed