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

Write out reports to individual reconstruction derivative folders #53

Merged
merged 51 commits into from
Aug 16, 2024

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Aug 14, 2024

Closes #51.

Changes proposed in this pull request

  • Create prep-style io_spec.json to control output filenames.
  • Replace ReconDerivativesDataSink with DerivativesDataSink for all non-folder outputs.
  • Start adding figures to HTML reports.
  • Create clean_datasinks function to define the output directory for each reconstruction workflow separately.
  • Refactor the AMICO workflow a bit. Sorry, I got carried away with indentation and such.

To do:

  • Add HTML summaries to reports. I need to figure out what info is relevant.

@tsalo tsalo marked this pull request as draft August 14, 2024 20:32
@tsalo tsalo added the bug Something isn't working label Aug 16, 2024
@tsalo
Copy link
Member Author

tsalo commented Aug 16, 2024

My PNG-to-SVG conversion is not working very well, unfortunately. I'll probably drop that in favor of opening an issue on Nireports.

EDIT: I've opened nipreps/nireports#124.

@tsalo tsalo changed the title Improve reports Write out reports to individual reconstruction derivative folders Aug 16, 2024
@tsalo tsalo marked this pull request as ready for review August 16, 2024 20:46
@tsalo tsalo requested a review from mattcieslak August 16, 2024 20:47
Copy link
Member Author

Choose a reason for hiding this comment

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

The new interfaces here aren't used yet. I want to tackle that in a separate PR.

@tsalo
Copy link
Member Author

tsalo commented Aug 16, 2024

I still want to see if I can reorganize the scalar datasink into a short workflow at some point, but the TSV splitter datasink and folder-based recon datasink will be harder.

@mattcieslak
Copy link
Contributor

I always prefer workflows too. I ended up with interfaces for these because you can't predict ahead of time what might be produced in the different nodes. But there might be a way! :roadmap:

Copy link
Contributor

@mattcieslak mattcieslak 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 brilliant work. Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like a massive improvement

Comment on lines +47 to +61
DIFFUSION_TEMPLATE = """\t\t<h3 class="elem-title">Summary</h3>
\t\t<ul class="elem-desc">
\t\t\t<li>Phase-encoding (PE) direction: {pedir}</li>
\t\t\t<li>Susceptibility distortion correction: {sdc}</li>
\t\t\t<li>Coregistration Transform: {coregistration}</li>
\t\t\t<li>Denoising Method: {denoise_method}</li>
\t\t\t<li>Denoising Window: {denoise_window}</li>
\t\t\t<li>HMC Transform: {hmc_transform}</li>
\t\t\t<li>HMC Model: {hmc_model}</li>
\t\t\t<li>DWI series resampled to spaces: T1wACPC</li>
\t\t\t<li>Confounds collected: {confounds}</li>
\t\t\t<li>Impute slice threshold: {impute_slice_threshold}</li>
\t\t</ul>
{validation_reports}
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

this is only relevant for preprocessing. We'll have to get creative for how to represent recon workflows here

@mattcieslak mattcieslak merged commit 60a5596 into main Aug 16, 2024
17 checks passed
@mattcieslak mattcieslak deleted the improve-reports branch August 16, 2024 21:19
@tsalo tsalo mentioned this pull request Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_dipy_mapmri outputs folders with node name and node qsirecon_suffix
2 participants