-
Notifications
You must be signed in to change notification settings - Fork 587
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
Surface plots #2477
Surface plots #2477
Conversation
The reason is use check_mesh also in plot_img_on_surf
Instead of popping keyword arguments from a kwargs dictionary, add options directly as individual keyword args to plot_img_on_surf
Docs where missing or incomplete in surface.check_mesh and plotting.surf_plotting
Co-Authored-By: dangom <d.gomez@donders.ru.nl>
Co-Authored-By: dangom <d.gomez@donders.ru.nl>
Codecov Report
@@ Coverage Diff @@
## master #2477 +/- ##
==========================================
+ Coverage 86.66% 86.76% +0.09%
==========================================
Files 99 99
Lines 12584 12660 +76
Branches 2418 2437 +19
==========================================
+ Hits 10906 10984 +78
+ Misses 1070 1068 -2
Partials 608 608
Continue to review full report at Codecov.
|
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 to all contributors !
looks good @ZviBaratz ! could you have a look at the codecov/patch and try to cover some of the cases that are missed in the tests? |
Absolutely. I'll update the PR this weekend. |
cool, thanks! |
Super cool! Thanks to all the contributors. This is going to be a big feature. Could you add an entry to doc/whats_new.rst with a link to the example that showcases this functionality. |
Hum, "one last thing": it seems that, on certain systems at least, the aspect ratio of the plot is not correct: Could you have a look to see how to fix that. Thanks a lot! |
Alright, so just to summarize:
@GaelVaroquaux could you please elaborate on the aspect ratio issue? I think everything is rendering fine for me. I'm juggling a number of things this week, so I hope I manage to make progress with the tests and aspect ratio, but in any case I'll take care of the documentation updates soon. |
In the example that you find in the generated documentation above, the images do not have the right aspect ratio: they are squeezed horizontally, which creates an ugly deformation of the meshes. There is probably somewhere a wrong resizing of the plot that should be cleaned. |
Dear @dangom, |
@bthirion There are automated formatters & linters as well as cloud based code quality platforms that would automate a lot of minutia, making it a non-issue for contributors & reviewers. We should start incorporating those. Nilearn contributors, managers, core developers juggle a lot of other responsibilities. We should automate the stuff that we can. |
Opened an issue #2528 to take off some of the load. |
Thanks for bringing this up, @dangom ! I'm hesitating to post this here only since I worry it takes some of the focus of @ZviBaratz's efforts, but I also think this is an important discussion that makes sense to closely link with this PR's lifecycle.
In general, I think most of the team is agreed that we want a low barrier to contribution. In cases like this, I see this as a question of "Should X be merged into master if we know if needs follow-up work?" From my own experience with the project, the presiding sentiment is that PRs should be largely complete to be merged, likely for the reason that @bthirion identified, which is a fear that if things are not complete at merge, they will not be addressed later. As a volunteer-driven project (at least at present), I think that's a reasonable fear. I have found that there are exceptions made to this procedure for core developers, which creates confusion around what is and isn't standard process. Building on @dangom's idea of minimal contribution guidelines: there are currently no written guidelines for when features are and are not considered, or are or are not ready to be merged. Minimal contribution guidelines would be a step in this direction, and I know there's more that we can do. There's also the other angle @kchawla-pi points out of automating away some of these issues. There are places this can be done, and I'm happy to continue discussing that in #2528, but I think that many of the concerns (as I'm reading them) can't be automated away, but could be ameliorated through improved contributing guidelines and decision-related documentation. |
the name of a matplotlib or nilearn colormap. | ||
""" | ||
|
||
cbar_vmin, cbar_vmax, vmin, vmax = _get_colorbar_and_data_ranges( |
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.
you can use cbar_vmin and cbar_vmax for symmetric_cbar to have an effect (or not expose a symmetric_cbar parameter)
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'm not sure I understand exactly what you mean, please see the updated code and elaborate in case any changes are still required.
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 meant that as we are not taking it into account, we should probably remove the symmetric_cbar
parameter (as I saw you have already done for plot_img_on_surf
, thanks).
My comment on cbar_vmin
is that if we wanted to actually have a symmetric_cbar
argument (we might add it in a future PR), we could get the necessary information from cbar_vmin
and cbar_vmax
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'm sorry but I'm still not sure I understand - symmetric_cbar
is elsewhere used in _colorbar_from_array()
and plot_surf_stat_map()
. Would you like for me to remove it in both? They both call _get_colorbar_and_data_ranges()
, which is imported from the img_plotting
module and requires this argument. If any one of the two calling functions omits the symmetric_cbar
parameter we should probably also give it a default of "auto".
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.
when _get_colorbar_and_data_ranges
is called with symmetric_cbar=False
or "auto" resolves to False
, it takes that into account by returning appropriate values for cbar_vmin
and cbar_vmax
. at the moment _colorbar_from_array
ignores the values of cbar_vmin
and cbar_vmax
, so the value of symmetric_cbar
has no effect, _colorbar_from_array
always behaves as if symmetric_cbar=True
. So my suggestion if (at least for now) to remove symmetric_cbar
as a parameter from _colorbar_from_array
's signature, as it has no effect anyway, and always call _get_colorbar_and_data_ranges(... , symmetric_cbar=True)
. A future PR can add this parameter and handle it corrrectly, but the new functionality is still useful without it.
regarding the call to plot_surf_stat_map
, the value of symmetric_cbar
doesn't matter because we call it with cbar=False
: it does not add a colorbar here, instead plot_img_on_surf
adds a colorbar for the whole figure separately.
so to summarize I suggest: no symmetric_cbar
parameter for plot_img_on_surf
nor _colorbar_from_array
, and _colorbar_from_array
calls _get_colorbar_and_data_ranges
with symmetric_cbar=True
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.
Had to pass symmetric_cbar=True
as a positional argument (avoiding changing anything in the img_plotting
module) but otherwise I hope the updated code reflects what you said.
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.
yes that is what I meant, thank you for bearing with me
thanks again for all your work @dangom and @ZviBaratz, sorry if there Adding documentation on how to write examples and improving contribution |
Thanks to @ZviBaratz and @dangom for pushing this work further. I really understand the frustration of the work (I do think that "frustration" is the right term). From the contributors' perspective, it can feel like there are useless barriers. From the maintainers' perspective, there is the constant worry that the code added to the library end up being a liability (I've written a bit about those two points of view here, though most of the post does not apply here). The worry about crushing a library on the weight of added code is real. Merging a PR that significantly adds run time to the CI will make every next move on the library harder. Unfortunately, too often contributors cannot find time to work on reducing such cost after the code is merged. It is also hard to find other people to do it: it's hard to work on someone else's code, in particular if it is to make it faster, or more robust (as opposed to adding features). However, they are additional things that we don't do well in the current process. First: feature creep in PRs. It is easy to get carried over, and always want more. This is exhausting, and we need to identify the minimal changes that can be merging. Second, we are slow at giving feedback, and slow at making decisions. This is due to the lack of core devs (we need to onboard more core devs), and also to our fear of making mistakes: by now, nilearn has so many users that a small mistake comes back as big complaints. Yet, we can and should do better!! @dangom : does that somewhat give a explanation of the situation that you feel can understand, even if you'd like the situation to be different (and I would too)? In the mean time, let's merge this PR! :) |
…d updated tests and docstrings for these functions.
…fter running the relevant test even if it fails.
…nd multiply itself by len(hemispehres) when determining figsize.
the sphinx template not found error is not due to this pr, you can ignore it |
Hi @ZviBaratz , if we decide to leave aside the remaining comments about the tests (I'm ok with that), I think all that remains is checking out |
thanks! the travis and azure failures are unrelated and fixed in #2530 and the circleci one is a download failure |
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! will merge in a couple of days unless someone eg @bthirion has more comments
merging with @bthirion 's approval. thanks again! |
Hi again,
This PR replaces #2454 which is a continuation of #1730 submitted by @dangom to resolve #1722. The intention is to check that everything other than the
symmetric_cbar
is working properly and merge the surface montages into master. I'll open an issue for thesymmetric_cbar
and fix that separately.Also, thank you so much @GaelVaroquaux and @effigies for your help creating the PR properly.