Skip to content

ENH: carpet plot tweaks #617

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Nov 3, 2021
Merged

ENH: carpet plot tweaks #617

merged 8 commits into from
Nov 3, 2021

Conversation

@notZaki
Copy link
Contributor

@notZaki notZaki commented Feb 9, 2021

This PR is a mix of fixes and enhancements to viz.plot_carpet. Most of the enhancements are based on personal preference so they can be dropped if desired.

Fixes

  • Legend should now show up if legend = True
    • The legend argument was not getting passed from carpet_plot() to _carpet()
  • Title should now show up if title argument is specified

Enhancements

  • The nskip argument should now work for NIfTI inputs
    • Looks like it's trivial to implement this for CIFTI as well, but I don't use them, so I didn't touch it
  • Tick formatting
    • If the xticks are frame numbers, then they will be printed as integers instead of decimals
    • Xticks now use the default font size
      • This prevents a clash with user-defined matplotlib configuration
  • The input data can now be a nibabel-image. A path (as a string) is still acceptable.
    • This is convenient because I can pre-process the data and then plot it directly without the i/o overhead

@pep8speaks
Copy link

pep8speaks commented Feb 9, 2021 •
edited
Loading

Hello @notZaki, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, pip install flake8 and then run flake8 niworkflows.

Comment last updated at 2021-11-03 18:24:10 UTC

@codecov
Copy link

codecov bot commented Feb 9, 2021 •
edited
Loading

Codecov Report

Merging #617 (2fce70b) into master (c2d7fad) will decrease coverage by 1.14%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #617      +/-   ##
==========================================
- Coverage   48.88%   47.73%   -1.15%     
==========================================
  Files          45       44       -1     
  Lines        5497     5489       -8     
  Branches      789      788       -1     
==========================================
- Hits         2687     2620      -67     
- Misses       2710     2776      +66     
+ Partials      100       93       -7
Flag Coverage Δ
masks ?
reportlettests 100.00% <ø> (ø)
unittests 47.73% <0.00%> (-0.07%) ⬇️

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

Impacted Files Coverage Δ
niworkflows/viz/plots.py 6.99% <0.00%> (-0.12%) ⬇️
niworkflows/func/util.py 17.97% <0.00%> (-31.47%) ⬇️
niworkflows/interfaces/header.py 51.70% <0.00%> (-11.12%) ⬇️
niworkflows/interfaces/fixes.py 50.00% <0.00%> (-5.56%) ⬇️
niworkflows/interfaces/reportlets/base.py 42.85% <0.00%> (-1.59%) ⬇️
niworkflows/__init__.py
niworkflows/viz/utils.py 8.87% <0.00%> (+0.02%) ⬆️
niworkflows/interfaces/norm.py 25.86% <0.00%> (+0.14%) ⬆️
niworkflows/interfaces/images.py 66.88% <0.00%> (+0.21%) ⬆️
niworkflows/utils/bids.py 75.00% <0.00%> (+0.74%) ⬆️

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 c2d7fad...2fce70b. Read the comment docs.

@oesteban oesteban self-assigned this May 7, 2021
@oesteban
Copy link
Member

oesteban commented May 7, 2021

Sorry for our slow turn around @notZaki - I'll go through this next week.

oesteban
oesteban reviewed Nov 3, 2021
View reviewed changes
niworkflows/viz/plots.py
nskip : int, optional
Number of volumes at the beginning of the scan marked as nonsteady state.
Not used.
Only used by volumetric NIfTI.
Copy link
Member

@oesteban oesteban Nov 3, 2021

Choose a reason for hiding this comment

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

@mgxd I'm realizing of an important issue with CIFTIs - are we dropping nonsteady states before sampling?

oesteban
oesteban reviewed Nov 3, 2021
View reviewed changes
@oesteban oesteban merged commit e179f92 into nipreps:master Nov 3, 2021
@oesteban oesteban mentioned this pull request Nov 4, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oesteban oesteban oesteban left review comments

Assignees

@oesteban oesteban

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@notZaki @pep8speaks @oesteban