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.

Sign up for GitHub

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

Jump to bottom

Add a preferred_cmap attribute to visual.py #2131

Merged
merged 1 commit into from
Aug 5, 2020
Merged

Add a preferred_cmap attribute to visual.py #2131

merged 1 commit into from
Aug 5, 2020

Conversation

kakirastern
Copy link

Description

To enable the addition of a preferred_cmap attribute so that if this parameter is set a matplotlib.colors.Colormap object can be used as the colormap setting of the 2D image.

@kakirastern kakirastern changed the title Draft to add a preferred_cmap attribute to visual.py [WIP] Draft to add a preferred_cmap attribute to visual.py May 5, 2020
@kakirastern kakirastern changed the title [WIP] Draft to add a preferred_cmap attribute to visual.py Draft to add a preferred_cmap attribute to visual.py May 6, 2020
@kakirastern
Copy link
Author

Finally got every working the way @astrofrog and @Cadair envisioned it while passing Travis CI! PR is ready for a review.

@kakirastern
Copy link
Author

@astrofrog Would it be a good idea to add something similar for the percentile_display and stretch_display parameters for setting the "norm"?

@codecov
Copy link

codecov bot commented May 6, 2020 •
edited
Loading

Codecov Report

Merging #2131 into master will decrease coverage by 0.01%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2131      +/-   ##
==========================================
- Coverage   87.86%   87.85%   -0.02%     
==========================================
  Files         246      246              
  Lines       22709    22721      +12     
==========================================
+ Hits        19954    19961       +7     
- Misses       2755     2760       +5
Impacted Files Coverage Δ
glue/core/registry.py 100.00% <ø> (ø)
glue/core/visual.py 85.00% <90.90%> (+0.55%) ⬆️
glue/core/state.py 92.12% <100.00%> (+0.02%) ⬆️
glue/viewers/image/state.py 91.07% <100.00%> (ø)
glue/viewers/matplotlib/qt/widget.py 73.86% <0.00%> (-7.96%) ⬇️
glue/core/data_factories/image.py 95.12% <0.00%> (+7.31%) ⬆️

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 d172634...381fe75. Read the comment docs.

astrofrog
astrofrog requested changes May 14, 2020
View reviewed changes
Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! As indicated below, can you revert all changes in the .glu files and make sure everything still works?

glue/core/visual.py Show resolved Hide resolved
glue/plugins/dendro_viewer/qt/tests/data/dendro_v0.glu Outdated Show resolved Hide resolved
doc/customizing_guide/mpl_viewer/config.py Outdated Show resolved Hide resolved
astrofrog
astrofrog requested changes May 15, 2020
View reviewed changes
Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates! Just a few more requests 😄

glue/viewers/image/state.py Outdated Show resolved Hide resolved
glue/viewers/image/state.py Outdated Show resolved Hide resolved
glue/viewers/matplotlib/state.py Outdated Show resolved Hide resolved
doc/customizing_guide/mpl_viewer/config.py Outdated Show resolved Hide resolved
@kakirastern
Copy link
Author

Think I will need some help to debug the Linux-related CI checks failures...

@kakirastern
Copy link
Author

Plus one macos-related CI test failure...

@kakirastern
Copy link
Author

Locally pytest is running fine though

astrofrog
astrofrog requested changes May 28, 2020
View reviewed changes
Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there - just have a couple of small remaining comments. Thanks!

glue/core/visual.py Outdated Show resolved Hide resolved
glue/viewers/image/state.py Outdated Show resolved Hide resolved
astrofrog
astrofrog requested changes May 28, 2020
View reviewed changes
Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final round of comments 😀

glue/viewers/image/state.py Outdated Show resolved Hide resolved
glue/viewers/image/state.py Outdated Show resolved Hide resolved
glue/viewers/matplotlib/state.py Outdated Show resolved Hide resolved
@kakirastern
Copy link
Author

Changes made. PR is ready for another review.

@kakirastern kakirastern changed the title Draft to add a preferred_cmap attribute to visual.py Add a preferred_cmap attribute to visual.py Jun 21, 2020
astrofrog
astrofrog requested changes Jul 13, 2020
View reviewed changes
Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of small comments below, but this is almost good to go!

glue/core/visual.py Outdated Show resolved Hide resolved
glue/viewers/matplotlib/state.py Outdated Show resolved Hide resolved
astrofrog
astrofrog requested changes Jul 15, 2020
View reviewed changes
Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now but see small comment below

glue/viewers/image/qt/slice_widget.py Outdated Show resolved Hide resolved
astrofrog
astrofrog approved these changes Jul 20, 2020
View reviewed changes
Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for responding to the reviews 😄

@kakirastern
Copy link
Author

My pleasure 🙂

Debug to make the new preferred_cmap attribute of visual to work
381fe75 
Edit files to remove unused imports

Comment out problematic set_preferred_cmap line

Get rid of unneeded set_preferred_cmap lines

Update docstring of preferred_cmap callback property in visual

Clean up after conducting more thorough testing

Fix typo in doctring

Tidy up and debug

Remove preferred_cmap as a DeferredDrawCallbackProperty

Remove unused line

Add preferred_cmap attribute for SunPy SJI maps

Tidy up by removing print statement for testing

Make changes according to reviewer suggestion

Make changes according to reviewer's suggestions

Make changes according to reviewer's suggestions

Make modifications according to reviewer suggestions
@astrofrog astrofrog merged commit 1fe8aea into glue-viz:master Aug 5, 2020
@kakirastern kakirastern deleted the add-preferred-colormap-to-visual branch August 5, 2020 12:12
@kakirastern
Copy link
Author

Thanks @astrofrog!

@dhomeier dhomeier mentioned this pull request May 2, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@astrofrog astrofrog astrofrog approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kakirastern @astrofrog