DOC: Fixes the AFQ tract profile tutorial. #3178
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3178 +/- ##
==========================================
- Coverage 83.75% 83.61% -0.15%
==========================================
Files 153 153
Lines 21343 21285 -58
Branches 3445 3438 -7
==========================================
- Hits 17876 17797 -79
- Misses 2611 2630 +19
- Partials 856 858 +2
|
There was a problem hiding this comment.
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?
Apart from this 2 points, it looks good to go.
|
The result looks correct. I'll look into the tqdm issue and report back. |
dipy/data/fetcher.py
Outdated
| return data_files | ||
|
|
||
|
|
||
| def fetch_hbn(subjects, path=None, include_afq=False): |
There was a problem hiding this comment.
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):
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
if it breaks, put it back as it was before, I plan a decorator to handle this and warn user for the whole codebase.
|
Hi @arokem,
Do you have an update on this ? |
|
Hi @arokem, It would be great if you could finalize this PR. It seems very close to go! Thanks |
|
TBH, I am not sure what remains here. You mentioned that it takes a long time to download the data, and TQDM not showing up. The time is because this downloads a large amount of diffusion MRI and derivatives, and I can't replicate TQDM not showing up on my end. |
|
Would it be helpful if we download only the bits of the data that are directly used here maybe? I can see pros and cons to that. |
I am ok with the long time to download. it was the TQDM issue. Let me try again to see if TQDM shows up. |
| "bundles_cst.left.trk") | ||
| af_l_file = op.join(bundles_folder, "bundles_2_subjects", "subj_2", "bundles", | ||
| "bundles_af.left.trk") | ||
| fdict, path = fetch_hbn([subject], include_afq=True) |
There was a problem hiding this comment.
fetch_hbn is not imported so the tutorial fails
There was a problem hiding this comment.
Fixed in last series of commits
|
Can you also address the code format issue. See https://github.com/dipy/dipy/actions/runs/8715096640/job/23906448911?pr=3178#step:4:130 This is just spacing issue. Thanks ! |
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.
|
Hello @arokem, Thank you for updating ! Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated at 2024-05-16 16:54:23 UTC |
arokem
force-pushed
the
fix_tract_profiles_example
branch
from
2b37c04
to
00a4758
Compare
|
Now rebased on top of main and with ruff formatting applied. |
|
Thank you for the update @arokem. Merging since codespell and code format are ok, we do not have CI's for examples yet. |
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.