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

ENH: Refactor mosaic plot custom colormap creation #151

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jhlegarreta
Copy link
Collaborator

Refactor the mosaic plot custom colormap creation to a separate method. Create a matplotlib ListedColormap instance given a colormap name and a maximum alpha value. Avoids accessing the colormaps instance's private _init() method and _lut attribute.

@jhlegarreta jhlegarreta requested a review from effigies January 8, 2025 14:14
@jhlegarreta jhlegarreta force-pushed the RefactorColormapCreation branch 3 times, most recently from a867eaa to 3412411 Compare January 8, 2025 14:31
Copy link

codecov bot commented Jan 8, 2025

Codecov Report

Attention: Patch coverage is 22.22222% with 7 lines in your changes missing coverage. Please review.

Project coverage is 65.47%. Comparing base (383c913) to head (eeaef58).

Files with missing lines Patch % Lines
nireports/reportlets/mosaic.py 22.22% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #151      +/-   ##
==========================================
+ Coverage   65.25%   65.47%   +0.21%     
==========================================
  Files          25       25              
  Lines        2674     2679       +5     
  Branches      420      420              
==========================================
+ Hits         1745     1754       +9     
+ Misses        814      813       -1     
+ Partials      115      112       -3     
Flag Coverage Δ
unittests 64.86% <22.22%> (?)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@effigies
Copy link
Member

effigies commented Jan 8, 2025

Do you have a demonstration of equivalence? I believe this functionality is only used in nirodents, which doesn't yet use nireports, so there's a possibility of introducing bugs or unintended changes that won't be noticed for some time.

cc @eilidhmacnicol @oesteban

@jhlegarreta
Copy link
Collaborator Author

OK, looks like I was too fast willing to address this. The ._lut attribute can only be obtained calling ._init(), and a ListedColormap instance does provide one, which makes sense. However, after some investigation, I've come up with the following two potential solutions that provide values that are close to those used in the current implementation:
https://gist.github.com/jhlegarreta/ece0b9f65443d89d8e2c804e185726bf

The function create_cmap_public_api_segment_data:

  • Provides RGB values that match exactly with the current implementation, but alpha values do not, i.e. the interpolation points are not those that would give the answer of the current implementation. If anyone has an idea of which points should be selected, please go ahead. Otherwise, if this is not a critical aspect, a possibility to get around this would be to accept the values be different in the new implementation. For any given colormap, I guess the statement mpl._cm.datad["Reds"] would require some sort of # ignore[...] to make the linter (?) happy about importing a private module (?).

    Additionally, the current implementation does not modify the cmap._segmentdata.alpha array (which in the above method is provided through the cdict dictionary) and the _rgba_bad, _rgba_over, _rgba_under are not modified, and I am not sure how matplotlib decides whether to use the values over N vs. those attributes.

The function create_cmap_public_api:

  • Provides RGB values that match exactly with the current implementation except for the alpha values of the bad/over/under elements (last 3 elements of the 259-length ._lut attribute. Its ._segmentdata attribute contains 256 elements instead of the 9 used by the default colormap/current implementation

Refactor the mosaic plot custom colormap creation to a separate method.
Create a `matplotlib` `ListedColormap` instance given a colormap name
and a maximum alpha value. Avoids accessing the colormaps instance's
private `_init()` method and `_lut` attribute.
@jhlegarreta jhlegarreta force-pushed the RefactorColormapCreation branch from ad447bc to eeaef58 Compare February 8, 2025 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants