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

[ENH] Change default BundleDict behavior to resample #755

Merged
merged 2 commits into from
Oct 17, 2021

Conversation

36000
Copy link
Collaborator

@36000 36000 commented Oct 14, 2021

Users who specify a custom bundledict usually still want to resample, and might miss the resample_to parameter.

AFQ/api.py Outdated
@@ -92,7 +92,7 @@ class BundleDict(MutableMapping):
def __init__(self,
bundle_info=BUNDLES,
seg_algo="afq",
resample_to=False):
resample_to=afd.read_mni_template()):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This worries me a little bit. Having a function call in the signature of another function seems a bit risky. Doesn't this trigger the mni download on import? How about setting this to None and then have the default behavior be as follows?

if resample_to is None: 
    resample_to = afd.read_mni_template()

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or is that too confusing? Because None sounds like you are not resampling?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this triggers download on import, but I agree that making this None would be more in line with what we do elsewhere. I will make that change.

@arokem
Copy link
Collaborator

arokem commented Oct 17, 2021

LGTM! Merging.

@arokem arokem merged commit 20bc54c into yeatmanlab:master Oct 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants