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

Update docstring for the aggregate_per_slice_or_level function #3830

Merged
merged 2 commits into from Jul 5, 2022

Conversation

mguaypaq
Copy link
Member

@mguaypaq mguaypaq commented Jul 4, 2022

Description

Two small followup changes after #3823:

Linked issues

Fixes #3824.

These comments used to be necessary to silence a PyCharm code
inspection. The warning was caused by a test function argument being
named the same as a custom pytest fixture defined in the file (which
can't be helped: that's how pytest fixtures work). But from a quick
test on my laptop, it looks like PyCharm is now smart enough to
recognize this pattern, and doesn't warn about it anymore.
Following #3823, aggregating from vertebral levels respects the
restriction to a subset of slices, instead of superseding it.
@mguaypaq mguaypaq added refactoring category: improves code structure without affecting user-facing functionality SCT API: aggregate_slicewise.py context: labels Jul 4, 2022
@mguaypaq mguaypaq added this to the 5.7 milestone Jul 4, 2022
@mguaypaq mguaypaq self-assigned this Jul 4, 2022
@codecov
Copy link

codecov bot commented Jul 4, 2022

Codecov Report

Merging #3830 (d375baa) into master (6f09a1c) will not change coverage.
The diff coverage is n/a.

Flag Coverage Δ
api-tests 21.86% <ø> (ø)
cli-tests 58.76% <ø> (ø)

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

Impacted Files Coverage Δ
spinalcordtoolbox/aggregate_slicewise.py 94.84% <ø> (ø)

Copy link
Member

@joshuacwnewton joshuacwnewton left a comment

Choose a reason for hiding this comment

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

Thank you for taking care of these straightforward, easy changes! LGTM!

I've temporarily re-enabled the "Merge commit" and "Rebase merging" options, since they seem more suitable for these two well-organized, atomic commits than squash merging.

@mguaypaq mguaypaq merged commit 50f030f into master Jul 5, 2022
@mguaypaq mguaypaq deleted the mgp/801 branch July 5, 2022 16:46
@mguaypaq mguaypaq changed the title Followups for #3823 Update docstring for the aggregate_per_slice_or_level function Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring category: improves code structure without affecting user-facing functionality SCT API: aggregate_slicewise.py context:
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean up obsolete noinspection comments
2 participants