Surface plots

[ZviBaratz]

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 the symmetric_cbar and fix that separately.

Also, thank you so much @GaelVaroquaux and @effigies for your help creating the PR properly.

[codecov]

Codecov Report

Merging #2477 into master will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            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              

Impacted Files	Coverage Δ
nilearn/plotting/__init__.py	88.46% <ø> (ø)
nilearn/plotting/html_surface.py	100.00% <100.00%> (ø)
nilearn/plotting/surf_plotting.py	90.23% <100.00%> (+5.33%)	⬆️
nilearn/surface/surface.py	94.59% <100.00%> (+0.23%)	⬆️
nilearn/datasets/utils.py	67.08% <0.00%> (-0.51%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b36df65...6feb6ba. Read the comment docs.

[bthirion]

LGTM, thx to all contributors !

[jeromedockes]

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?
also, could you add the new function to /doc/modules/reference.rst under the plotting section so that it shows up in the documentation? you can then check the result with make doc to build the documentation without running the examples

[ZviBaratz]

Absolutely. I'll update the PR this weekend.

[jeromedockes]

cool, thanks!

[GaelVaroquaux]

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.

[GaelVaroquaux]

Hum, "one last thing": it seems that, on certain systems at least, the aspect ratio of the plot is not correct:
https://5679-1235740-gh.circle-artifacts.com/0/doc/_build/html/auto_examples/01_plotting/plot_3d_map_to_surface_projection.html#plot-multiple-views-of-the-3d-volume-on-a-surface

Could you have a look to see how to fix that. Thanks a lot!

[ZviBaratz]

Alright, so just to summarize:

• Create some more tests and confirm increased coverage.
• Add plot_img_on_surf() to /doc/modules/reference.rst.
• Add an entry in doc/whats_new.rst linking to the new example.
• Fix issue with aspect ratio.

@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.

[bthirion]

* Fix issue with aspect ratio

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.

[bthirion]

Dear @dangom,
Sorry if we've been unfair to you. There's is probably a lack of care on our side, in part due to the fact that our workforce are present for short -term positions with gap in between, making it hard to continuously track PRs and provide guidance and advice.
We're really thankful for your efforts, and it may well be the case that we don't provide useful enough feedback to integrate new contributions fast enough and keep the pace in the tough integration work.
There is probably a tradeoff to find, but looking at the past, it has happened quite often that people were moving to a different field or career, making it har to fix their contributions after they had left. I'm in the right position that this has introduced some issues in Nistats, for instance.
Now I pretty much like your suggestion to establish "minimal contribution" guidelines to accept PRs.
Best

[kchawla-pi]

@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.

[kchawla-pi]

Opened an issue #2528 to take off some of the load.

[emdupre]

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.

I agree that APIs changes may break downstream code, but for a convenience plotting function, could we rework "left+right" to ["left", "right"] or refactor check_mesh into a more general function after merging the PR? And could we update tests once we see issues with the plotting?

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.

[jeromedockes]

you can use cbar_vmin and cbar_vmax for symmetric_cbar to have an effect (or not expose a symmetric_cbar parameter)

[ZviBaratz]

I'm not sure I understand exactly what you mean, please see the updated code and elaborate in case any changes are still required.

[jeromedockes]

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

[ZviBaratz]

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".

[jeromedockes]

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

[ZviBaratz]

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.

[jeromedockes]

yes that is what I meant, thank you for bearing with me

[jeromedockes]

thanks again for all your work @dangom and @ZviBaratz, sorry if there
are many review comments to address. I think we are almost there, please
let me know if I can help out in any way.

Adding documentation on how to write examples and improving contribution
guidelines are good ideas.

[GaelVaroquaux]

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! :)

[jeromedockes]

the sphinx template not found error is not due to this pr, you can ignore it

[jeromedockes]

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 image.py from master and merging master (resolving conflicts) and this will be ready to merge. thanks again!

[jeromedockes]

thanks! the travis and azure failures are unrelated and fixed in #2530 and the circleci one is a download failure

[jeromedockes]

LGTM! will merge in a couple of days unless someone eg @bthirion has more comments

[jeromedockes]

merging with @bthirion 's approval. thanks again!