-
Notifications
You must be signed in to change notification settings - Fork 580
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
[ENH] Improve plotting contours for PlotlySurfaceFigure
objects by adding add_contours
method
#3949
base: main
Are you sure you want to change the base?
Conversation
👋 @psadil 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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3949 +/- ##
==========================================
+ Coverage 91.85% 92.84% +0.98%
==========================================
Files 144 147 +3
Lines 16419 17166 +747
Branches 3434 3611 +177
==========================================
+ Hits 15082 15938 +856
+ Misses 792 685 -107
+ Partials 545 543 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@psadil thanks for getting started on this! Will try to start a review this week |
PlotlySurfaceFigure
objects by adding add_contours
method
Tests added! Do these seem like they're on the right track? |
Thanks!!
Had a very quick look: it does help also understand how these methods are used, so definitely useful |
Co-authored-by: Remi Gau <remi_gau@hotmail.com>
Co-authored-by: Remi Gau <remi_gau@hotmail.com>
Co-authored-by: Remi Gau <remi_gau@hotmail.com>
Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>
Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>
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 !
Co-authored-by: Himanshu Aggarwal <himanshuaggarwal1997@gmail.com>
Co-authored-by: Remi Gau <remi_gau@hotmail.com>
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.
Thanks for resolving all the comments @psadil ! But just a few more tests :)
And then we can add this to the changelog, see: https://nilearn.github.io/stable/development.html#changelog
It looks like those failures for NPY201 are being triggered by different parts of the repo? |
yes that's not you I will merge the main branch in your PR to solve this: remember to pull first before you push again 😉 |
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.
Looks mostly good. A little effort on pedagogy would help.
|
||
# Loop over the remaining vertices in order of distance from the | ||
# current vertex | ||
while len(visited_vertices) < len(centroids): |
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.
Can we make sure that the loop ends ?
What algorithm is implemented here ?
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.
It's a modification of the approach listed in the original issue: #3711 (comment), with the modifications aiming to ensure that the contour does not cross itself. I'm not sure how it could fail to end, but if preferred I think I could update it to a for loop.
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'd more comfortable with a for loop indeed...
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.
Sounds good! I cleaned up the sorting procedure a bit and added a few more explanatory comments cb8ad06.
Changes proposed in this pull request:
add_contours
method toPlotlySurfaceFigure
, increasing customizability of contour lines.