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: Bring implementation details to the foreground of documentation #248

Merged
merged 9 commits into from
Oct 27, 2021

Conversation

oesteban
Copy link
Member

@oesteban oesteban commented Oct 26, 2021

Trying to give some more visibility to the theory. Also, discussed a little about find_estimators (related to nipreps/fmriprep#2560)

EDIT: Link to the new document - https://3553-189236569-gh.circle-artifacts.com/0/%7E/docs/docs_refactor-content-organization/methods.html

@codecov-commenter
Copy link

codecov-commenter commented Oct 26, 2021

Codecov Report

Merging #248 (bc7ccda) into master (0e19395) will decrease coverage by 0.41%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #248      +/-   ##
==========================================
- Coverage   83.06%   82.64%   -0.42%     
==========================================
  Files          25       25              
  Lines        1836     1902      +66     
  Branches      208      231      +23     
==========================================
+ Hits         1525     1572      +47     
- Misses        280      299      +19     
  Partials       31       31              
Flag Coverage Δ
travis 82.64% <100.00%> (-0.42%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
sdcflows/workflows/fit/syn.py 48.52% <ø> (ø)
sdcflows/workflows/fit/fieldmap.py 98.43% <100.00%> (ø)
sdcflows/workflows/fit/pepolar.py 100.00% <100.00%> (ø)
sdcflows/conftest.py 0.00% <0.00%> (ø)
sdcflows/utils/wrangler.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e19395...bc7ccda. Read the comment docs.

@oesteban oesteban force-pushed the docs/refactor-content-organization branch from 496b7f2 to 518d7f9 Compare October 26, 2021 20:22
@oesteban oesteban marked this pull request as ready for review October 27, 2021 09:40
Copy link
Contributor

@eilidhmacnicol eilidhmacnicol left a comment

Choose a reason for hiding this comment

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

Just some small things from me

docs/methods.rst Outdated Show resolved Hide resolved
docs/methods.rst Outdated Show resolved Hide resolved
docs/methods.rst Outdated Show resolved Hide resolved
@oesteban
Copy link
Member Author

Thanks much @eilidhmacnicol . Do you think you learned something? - this is just a start, but someone who reads it should end with a relatively clear idea of what's going on and how SDCFlows attacks the problem.

@eilidhmacnicol
Copy link
Contributor

Yes - as someone fairly representative of the target audience, the individual workflow functionalities were clear. There were some sentences that required a second read but I think, like you said, that it's a solid base.

@oesteban
Copy link
Member Author

@effigies - if the docs do not clarify the answers to your question #247 (comment), then I need help improve this text.

@mgxd - I believe this documentation should help a lot in identifying when a bug could come from (thinking of that weird flip in the direction of correction of your data). Also interested in your feedback.

@effigies
Copy link
Member

Will try to read through before our meeting.

Copy link
Contributor

@mgxd mgxd left a comment

Choose a reason for hiding this comment

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

This is super useful, definitely helpful to digest some of what is going on during SDC

docs/methods.rst Outdated Show resolved Hide resolved
docs/methods.rst Outdated Show resolved Hide resolved
docs/methods.rst Outdated Show resolved Hide resolved
docs/methods.rst Show resolved Hide resolved
docs/methods.rst Outdated Show resolved Hide resolved
docs/methods.rst Show resolved Hide resolved
@oesteban oesteban force-pushed the docs/refactor-content-organization branch from 981538b to 8726e6e Compare October 27, 2021 17:21
Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Looks good. Style issues only.

docs/methods.rst Outdated Show resolved Hide resolved
docs/methods.rst Outdated Show resolved Hide resolved
docs/methods.rst Outdated Show resolved Hide resolved
docs/methods.rst Outdated Show resolved Hide resolved
docs/methods.rst Outdated Show resolved Hide resolved
docs/methods.rst Outdated Show resolved Hide resolved
docs/methods.rst Outdated Show resolved Hide resolved
Co-authored-by: Chris Markiewicz <[email protected]>
@oesteban oesteban marked this pull request as draft October 27, 2021 19:52
@oesteban oesteban marked this pull request as ready for review October 27, 2021 20:40
@oesteban oesteban merged commit c802b4d into master Oct 27, 2021
@oesteban oesteban deleted the docs/refactor-content-organization branch October 27, 2021 20:41
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.

5 participants