-
Notifications
You must be signed in to change notification settings - Fork 576
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
Example to demonstrate copy_header_from
in math_img
#4392
Conversation
👋 @man-shu Thanks for creating a PR! Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft. Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.
For new features:
For bug fixes:
We will review it as quick as possible, feel free to ping us with questions if needed. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4392 +/- ##
==========================================
+ Coverage 91.85% 91.99% +0.13%
==========================================
Files 144 145 +1
Lines 16419 16709 +290
Branches 3434 3551 +117
==========================================
+ Hits 15082 15371 +289
+ Misses 792 767 -25
- Partials 545 571 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
The same MSDL dataset download fails during building the docs. So maybe first merging #4390 would be better. |
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.
The example LGTM.
I'm still not 100% sure that this deserves a full example, but I find it quite pedagogical, so I'm +0 for this addition.
Indeed, it has to come after #4390.
Yes, maybe it doesn't make much sense to have it for just one function. But as explained in #4393, I identified the same issue in several others under the |
Can you propose an enhanced example ? |
Yes, but I should fix #4393 first. |
So, all the other
|
We should decide on this at a forthcoming coredev meeting. |
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.
LGTM overall.
Co-authored-by: bthirion <bertrand.thirion@inria.fr>
Co-authored-by: bthirion <bertrand.thirion@inria.fr>
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.
Just create the worresponding whatsnew entry.
@@ -0,0 +1,146 @@ | |||
""" | |||
Copying headers from input images with ``math_img`` |
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.
Moved it from 06_manipulating_images
to 07_advanced
as @Remi-Gau said in the dev meeting
This is also ready for review @Remi-Gau |
checking it now |
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.
Good to go
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.
LGTM, thx.
Merging, then. |
math_img
#4362Changes proposed in this pull request: