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

[FIX] reorganize anat filename templates #1419

Merged
merged 7 commits into from
Mar 6, 2023

Conversation

Remi-Gau
Copy link
Collaborator

@Remi-Gau Remi-Gau commented Feb 14, 2023

fixes #1418

  • split filename templates macro for anat into:
    • parametric suffixes
    • defacemask
    • non parametric suffixes
  • add list of file collection suffixes in qMRI appendix

@Remi-Gau Remi-Gau marked this pull request as ready for review February 14, 2023 15:16
@Remi-Gau Remi-Gau requested a review from tsalo February 14, 2023 15:17
@codecov
Copy link

codecov bot commented Feb 14, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.23 🎉

Comparison is base (134f532) 88.65% compared to head (6e7df7b) 88.88%.

❗ Current head 6e7df7b differs from pull request most recent head 268dc7b. Consider uploading reports for the commit 268dc7b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1419      +/-   ##
==========================================
+ Coverage   88.65%   88.88%   +0.23%     
==========================================
  Files          11       11              
  Lines        1084     1080       -4     
==========================================
- Hits          961      960       -1     
+ Misses        123      120       -3     
Impacted Files Coverage Δ
tools/schemacode/bidsschematools/render/text.py 97.47% <0.00%> (ø)
tools/schemacode/bidsschematools/render/utils.py 85.18% <0.00%> (ø)
...ools/schemacode/bidsschematools/types/namespace.py 91.09% <0.00%> (ø)
tools/schemacode/bidsschematools/schema.py 77.90% <0.00%> (+2.35%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Remi-Gau
Copy link
Collaborator Author

@sappelhoff I labelled this as a fix but I am 100% sure this is accurate because the schema was correct but it was our rendering of it that created some ambiguity.

Hence not sure if this should actually go in the changelog.

@sappelhoff
Copy link
Member

thanks! I think it should go into the changelog, because what users mostly see is what we render.

@Remi-Gau
Copy link
Collaborator Author

thanks! I think it should go into the changelog, because what users mostly see is what we render.

good with me.
thanks!

Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

I like the overall change, but I have some suggestions to make it flow more nicely.

@Remi-Gau
Copy link
Collaborator Author

will have a closer look but it seems that I was being too conservative in my changes and that your suggestions really improve things.

Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

The changes look good to me. Thanks!

@effigies effigies self-assigned this Feb 23, 2023
@Remi-Gau
Copy link
Collaborator Author

@effigies
added some screenshots and links to the relevant HTML to help reviewing.

let me know if you need something else.

@effigies
Copy link
Collaborator

effigies commented Feb 27, 2023

Overall looks good, but I think I prefer the filename template to come before the suffix list. Other modalities continue to follow this convention, so I think it would make more sense to keep that as-is for now. A global reordering could be proposed separately (though I would vote against it).

@Remi-Gau
Copy link
Collaborator Author

Overall looks good, but I think I prefer the filename template to come before the suffix list. Other modalities continue to follow this convention, so I think it would make more sense to keep that as-is for now. A global reordering could be proposed separately (though I would vote against it).

good point

@Remi-Gau
Copy link
Collaborator Author

filename template to come before the suffix list.

@effigies

This is done.

@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Mar 6, 2023

it's been 5 days so I will merge this

@Remi-Gau Remi-Gau merged commit 9594922 into bids-standard:master Mar 6, 2023
@Remi-Gau Remi-Gau deleted the anat branch April 11, 2024 11:33
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.

[BUG] ambiguity in filename template in anat
4 participants