tedana
tedana copied to clipboard
Add section to reports that show system info, tedana call and version
Closes #650 and #687 .
Changes proposed in this pull request:
- [x] Add a function to return system info, then save it into a json file.
- [x] Save tedana call into a variable and add to the dictionary with system info.
- [x] Add tedana version into the dictionary with system info.
- [x] Get Python version and add it to the dictionary with system info.
This PR is pretty much ready for review but I would like to wait until #696 is merged to make some final changes for the sake of coherence.
@eurunuela this looks good but there's actually one potential downfall: if you call tedana
with the API instead of on the command line, I'm not sure you would get a good idea of what the API was called with. We may instead want to create a list of parameters that get captured from the API call and display those. We could also add some logic by having a new workflow variable that captures the mode as cli
at the __name__ == '__main__'
check and add logic to this PR to format appropriately for CLI/API. Happy to help with that.
We may instead want to create a list of parameters that get captured from the API call and display those. We could also add some logic by having a new workflow variable that captures the mode as
cli
at the__name__ == '__main__'
check and add logic to this PR to format appropriately for CLI/API.
I would be pretty happy with just a list/table/dict of parameters, rather than a reconstructed CLI call. Just my personal preference though.
@tsalo @jbteves do you like it now?
Codecov Report
Patch coverage: 100.00%
and project coverage change: +0.14%
:tada:
Comparison is base (
bf3392a
) 88.86% compared to head (83ffeff
) 89.00%.
Additional details and impacted files
@@ Coverage Diff @@
## main #747 +/- ##
==========================================
+ Coverage 88.86% 89.00% +0.14%
==========================================
Files 27 27
Lines 3368 3411 +43
Branches 618 622 +4
==========================================
+ Hits 2993 3036 +43
Misses 227 227
Partials 148 148
Files Changed | Coverage Δ | |
---|---|---|
tedana/reporting/html_report.py | 92.59% <100.00%> (+1.19%) |
:arrow_up: |
tedana/utils.py | 94.73% <100.00%> (+0.14%) |
:arrow_up: |
tedana/workflows/ica_reclassify.py | 97.92% <100.00%> (+0.13%) |
:arrow_up: |
tedana/workflows/tedana.py | 81.81% <100.00%> (+0.86%) |
:arrow_up: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I'm not entirely sure we want to display it CLI-style instead of printing a dict of parameters, but I'm guessing that it's easier to make pretty if we do it CLI-style and it should still be plenty legible. Plus, people could just copy and paste it to replicate.
I'm not entirely sure we want to display it CLI-style instead of printing a dict of parameters, but I'm guessing that it's easier to make pretty if we do it CLI-style and it should still be plenty legible. Plus, people could just copy and paste it to replicate.
I believe the way I put it it won't be exactly the same as the CLI but it should be pretty close.
Here's what it currently looks like for the four-echo test:

I really like it. I see that there are some issues with unset arguments (e.g., --prefix
followed by nothing, --mixm None
) that make me think that a table might be better, but it's still pretty interpretable. We may need a warning for naive users though. Just something telling them not to just copy and paste the command, since it's not exact.
One minor thing I'd love to see is a section title for the methods description, so that it's clear what the boundary is between command/system info and the report boilerplate.
Hm, would it be straightforward to do a simple table of each parameter getting a line? You could switch the format to
f"{key}: {value}<br>"
I think it's probably not great to have something that looks pastable but isn't, unfortunately...
Hm, would it be straightforward to do a simple table of each parameter getting a line?
We could create a table like the one below.
Re section title: that's coming on #696
I just noticed this PR has been sitting around and looks like it's close to merge-able, but still a draft. Anything critical to do before opening this up for review & merging?
Probably we should get the merge conflicts cleared up before reviewing.
I think this is ready. If anything, we may want to create the call.sh
file with the io_generator
object.
Any thoughts on that @tsalo @handwerkerd ?
Ha, I just realized the tests are failing because ica_reclassify
also uses the reports! I will add the system info to it too.
Hey @tsalo @handwerkerd, you might have missed the notification for this one.
This PR is ready for review now. Let me know if you would make any changes.
Could you copy over the changes to .circleci/config.yml
from #949 so we can look at the test outputs as well? It'll make it easier to check over the effects of your changes.
Could you copy over the changes to
.circleci/config.yml
from #949 so we can look at the test outputs as well? It'll make it easier to check over the effects of your changes.
Done in 11b9701.
How about this new design for the code block @tsalo?
I have moved the tedana command creation closer to the report, references and log creation.
One additional thing to consider is to add the version numbers of python modules that are most likely to affect results: mapca, nilearn, scipy, numpy, pandas, sklearn
I would leave the addition of these module versions for a future PR.