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

FIX: fix AttributeError Xtick has no attribute label #84

Merged
merged 3 commits into from
Dec 12, 2023

Conversation

celprov
Copy link
Collaborator

@celprov celprov commented Dec 11, 2023

No description provided.

Copy link

codecov bot commented Dec 11, 2023

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (01e9a93) 54.28% compared to head (844519f) 54.23%.

Files Patch % Lines
nireports/reportlets/nuisance.py 0.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #84      +/-   ##
==========================================
- Coverage   54.28%   54.23%   -0.05%     
==========================================
  Files          22       22              
  Lines        2019     2019              
  Branches      391      391              
==========================================
- Hits         1096     1095       -1     
  Misses        845      845              
- Partials       78       79       +1     
Flag Coverage Δ
unittests 54.23% <50.00%> (-0.05%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Suggestion. This mirrors nipreps/niworkflows#820. Assuming we also have confounds_correlation_plot and plot_melodic_components, we should update those as well.

nireports/reportlets/xca.py Outdated Show resolved Hide resolved
nireports/reportlets/xca.py Outdated Show resolved Hide resolved
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
@effigies
Copy link
Member

LGTM. Would it be reasonably quick to port over the unit tests, or should we push that off for the moment?

@effigies effigies merged commit 9f62aca into nipreps:main Dec 12, 2023
3 of 5 checks passed
@celprov
Copy link
Collaborator Author

celprov commented Dec 12, 2023

@effigies The three functions that I changed (compcor_variance_plot, plot_melodic_components and confounds_correlation_plot ) are seemingly already covered by tests here, here and here.

@effigies
Copy link
Member

Oh good. I didn't realize that the missing coverage was covered by the tests Taylor just added.

@celprov
Copy link
Collaborator Author

celprov commented Dec 13, 2023

@effigies, but why is codecov complaining that the new lines are not covered by the test then?

@effigies
Copy link
Member

Hmm. I thought the branches just weren't merged together, but it looks like the merge commit also didn't see these as covered: https://app.codecov.io/gh/nipreps/nireports/commit/9f62aca1759406934157255ebb4a66edd1bc8ea0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants