tedana icon indicating copy to clipboard operation
tedana copied to clipboard

Add section to reports that show system info, tedana call and version

Open eurunuela opened this issue 3 years ago • 12 comments

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.

eurunuela avatar Jul 10 '21 10:07 eurunuela

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 avatar Jul 10 '21 12:07 eurunuela

@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.

jbteves avatar Jul 12 '21 14:07 jbteves

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 avatar Jul 12 '21 21:07 tsalo

@tsalo @jbteves do you like it now?

eurunuela avatar Jul 15 '21 08:07 eurunuela

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.

codecov[bot] avatar Jul 15 '21 09:07 codecov[bot]

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.

jbteves avatar Jul 15 '21 14:07 jbteves

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.

eurunuela avatar Jul 15 '21 15:07 eurunuela

Here's what it currently looks like for the four-echo test:

Screen Shot 2021-07-15 at 12 09 44 PM

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.

tsalo avatar Jul 15 '21 16:07 tsalo

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...

jbteves avatar Jul 15 '21 16:07 jbteves

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

eurunuela avatar Jul 15 '21 16:07 eurunuela

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?

handwerkerd avatar Mar 17 '22 19:03 handwerkerd

Probably we should get the merge conflicts cleared up before reviewing.

jbteves avatar Mar 17 '22 19:03 jbteves

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 ?

eurunuela avatar Aug 09 '23 13:08 eurunuela

Ha, I just realized the tests are failing because ica_reclassify also uses the reports! I will add the system info to it too.

eurunuela avatar Aug 10 '23 09:08 eurunuela

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.

eurunuela avatar Aug 24 '23 12:08 eurunuela

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.

tsalo avatar Aug 24 '23 12:08 tsalo

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.

eurunuela avatar Aug 25 '23 07:08 eurunuela

How about this new design for the code block @tsalo?

eurunuela avatar Aug 28 '23 13:08 eurunuela

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.

eurunuela avatar Aug 30 '23 07:08 eurunuela