-
Notifications
You must be signed in to change notification settings - Fork 48
Generate graphic reports for phys2bids outputs #464
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #464 +/- ##
==========================================
+ Coverage 91.15% 91.75% +0.59%
==========================================
Files 8 9 +1
Lines 1006 1091 +85
==========================================
+ Hits 917 1001 +84
- Misses 89 90 +1
🚀 New features to boost your workflow:
|
|
@me-pic this is awesome!!! Can you look into the readthedocs failure, please? |
Co-authored-by: Katie Bottenhorn <[email protected]> Co-authored-by: Eneko Uruñuela <[email protected]> Co-authored-by: Vicente Ferrer <[email protected]> Co-authored-by: Taylor Salo <[email protected]> Co-authored-by: Stefano Moia <[email protected]>
for more information, see https://pre-commit.ci
|
@smoia Not sure exactly what is happening with the doc... Been able to build it locally without any error using sphinx-build. Any hint on what I could check ? |
This reverts commit f5473b5.
f5473b5 to
fd8beab
Compare
|
No idea - I think it's happening also in #462. It seems to be something about the phys2bids.phys2bids module import? |
|
@me-pic probably the best option is to update the documentation configuration (as well as sphinx packages version). Maybe we can open a new PR, trying to fit phys2bids docs into nigsp's configuration (it's more recent): https://github.com/MIPLabCH/nigsp/tree/master/docs |
3997ada to
10ab4ef
Compare
|
@smoia Thank you ! Will take a look next week. |
This reverts commit 8b3a336.
|
@me-pic do you still have problems with the docs? |
|
@smoia Yes.. I can't figure why, and I don't have any problem building it locally :( |
for more information, see https://pre-commit.ci
This reverts commit fe35cc0.
Co-authored-by: Basile Pinsard <[email protected]>
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.
I tested this with the data I'm using for the Repronim demo and it is mostly working - looks great! However, for this data there are four channels being saved, but only the first two generate plots in the report (the next two are completely missing). As well, I'm puzzled by the "phys2BIDS Output Directory" column. Is it meant to show the structure of the BIDS-ified files? Right now, it shows some assets (a bit confusing/distracting) and then the files in the folder that I ran phys2bids from (not the output directory or the files therein).
|
Can you post a screenshot please? |
|
@m-miedema do you still want changes on this PR? |
|
I don't think the issue with only the first two channel plots appearing was solved yet, but the output directory tree is better! |
|
@m-miedema regarding the plots including the first two channels, I believe the issue comes from the fact that the physio channels have different sampling frequency, and the So from there we could either change the function so that all the channels, regardless of their sampling frequency, will be plotted in the same report, or do different report for different sampling frequency. I think it will be better to have all the information at the same place and perhaps just adding the sampling frequency in the plot title (e.g. currently: @m-miedema @smoia WDYT ? I'll wait for your feedback on that before making the changes ! |
|
Crazy idea, but what if we had two reports that link to each others through the folder list? |
|
I think one report for every produced _physio file makes the most sense? |
for more information, see https://pre-commit.ci
|
@m-miedema @smoia I have made the changes to have one report per physio file (so one per sampling freq). @smoia I'm not completely sure how to link the reports to each other.. |


Closes #131, #407
The PR solved the issues in #407 with the proposed changes mentioned below.
The PR #407 was itself a follow-up of #243
Proposed Changes
Change Type
bugfix(+0.0.1)minor(+0.1.0)major(+1.0.0)refactoring(no version update)test(no version update)infrastructure(no version update)documentation(no version update)otherChecklist before review