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

DOC: Fixes the AFQ tract profile tutorial. #3178

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

arokem
Copy link
Contributor

@arokem arokem commented Apr 5, 2024

This also adds the capability to download AFQ derivatives for all of the HBN subjects from the FCP INDI bucket where those are stored as part of the HBN POD2 study.

Addresses #3175, hopefully once and for all.

This also adds the capability to download AFQ derivatives for all of the
HBN subjects from the FCP INDI bucket where those are stored as part of the
HBN POD2 study.
Copy link

codecov bot commented Apr 6, 2024

Codecov Report

Attention: Patch coverage is 3.84615% with 50 lines in your changes are missing coverage. Please review.

Project coverage is 83.61%. Comparing base (f2f72f0) to head (0f49459).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3178      +/-   ##
==========================================
- Coverage   83.65%   83.61%   -0.05%     
==========================================
  Files         153      153              
  Lines       21272    21285      +13     
  Branches     3434     3438       +4     
==========================================
+ Hits        17796    17797       +1     
- Misses       2618     2630      +12     
  Partials      858      858              
Files Coverage Δ
dipy/data/fetcher.py 41.23% <3.84%> (-0.60%) ⬇️

@skoudoro skoudoro linked an issue Apr 6, 2024 that may be closed by this pull request
Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Hi @arokem,

Thank you for this.

No sure the reason, but the download was quite long and TQDM did not show up. So I did not know if the terminal was freezing or something was happening.

Can you look at this, and make sure that TQDM shows up. Even when the data are already in place, we do not have any information like the other fetcher.

Otherwise, Can you tell me if this result looks correct to you?
image

Apart from this 2 points, it looks good to go.

@arokem
Copy link
Contributor Author

arokem commented Apr 9, 2024

The result looks correct. I'll look into the tqdm issue and report back.

return data_files


def fetch_hbn(subjects, path=None, include_afq=False):
Copy link
Member

Choose a reason for hiding this comment

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

Also, We start to introduce keyword-only arguments in the codebase.

can you replace this by def fetch_hbn(subjects, *, path=None, include_afq=False):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 5123bcd. Just noting that I've noticed that this transition is breaking downstream code. I guess folks will have to update all their calls if they want to update dipy.

Copy link
Member

Choose a reason for hiding this comment

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

if it breaks, put it back as it was before, I plan a decorator to handle this and warn user for the whole codebase.

Copy link
Member

Choose a reason for hiding this comment

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

sorry for that

@skoudoro
Copy link
Member

skoudoro commented Apr 16, 2024

Hi @arokem,

No sure the reason, but the download was quite long and TQDM did not show up. So I did not know if the terminal was freezing or something was happening.

Do you have an update on this ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tract profiles in afq example look all wrong
2 participants