-
Notifications
You must be signed in to change notification settings - Fork 190
[ENH] Expand fieldmaps section to include B1 maps, including qMRI maps #2183
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2183 +/- ##
=======================================
Coverage 82.71% 82.71%
=======================================
Files 20 20
Lines 1608 1608
=======================================
Hits 1330 1330
Misses 278 278 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Pinging @effigies @Remi-Gau and @agahkarakuzu for your opinions. |
|
Thank you @tsalo looks great to me! Early on this was premature, but now it is a timely change as qMRI concepts are less unfamiliar. |
|
I'm good with this. We probably need a little more text in the new sections (I haven't looked at the rendering, so feel free to refute me with a screenshot), but this makes sense to me. |
|
2 things:
I would suggest: moving table to the main MRI page under the relevant section (@agahkarakuzu why are some lines of that table empty - can we just remove the lines?) |
|
I agree that moving specification details out of the appendix and into the main spec is good. qMRI is a bit of an outlier in providing additional rules inside the appendix (and they are easy to skip over while working on the schema and/or validator). |
Co-authored-by: Remi Gau <[email protected]>
|
I tried to move the fieldmap-related information out of the appendix. I think it looks fairly good. One thing I just realized I don't know- is any metadata used to link B1 fieldmaps to specific scans? The sections on IntendedFor and B0FieldIdentifier (we'd want a B1FieldIdentifier I'm guessing) are nested under the B0 fieldmap section, and I don't know if I should move them. |
Co-authored-by: Chris Markiewicz <[email protected]>
Adding something now requires getting it right, so I would only do it if we're absolutely sure. We can clarify in the future. Instinctively, I'd say that we want to put a |
…-data.md" This reverts commit ada982f.
|
Yeah, looks like we need https://mrkeo.github.io/reference/mathjax/. I think it might be good to do, but out of scope for this PR. Will open an issue. |
bthirion
left a comment
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.
LGTM AFAICT
Remi-Gau
left a comment
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.
Good with me.
thanks @tsalo that's much better that anything I could have done!
|
This just reorganizes the documentation really, so @effigies do you think it merits a 5 day waiting period or should I just merge now? |
|
Let's merge. |
Closes #2181. This is just a first attempt. I'm happy to hand it over to someone else if anyone's interested.
Changes proposed: