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

Implement as_steps in profile layer state #2292

Merged
merged 2 commits into from
Apr 22, 2022

Conversation

kecnry
Copy link
Contributor

@kecnry kecnry commented Apr 19, 2022

Description

This implements a new boolean switch in the profile layer state to toggle displaying the profile as steps vs a line plot. The actual binning logic is adopted from https://github.com/astropy/specutils/blob/main/specutils/spectra/spectral_axis.py#L42. (binning logic has now been moved to the bqplot artist in glue-viz/glue-jupyter#309)

@kecnry kecnry marked this pull request as ready for review April 19, 2022 15:17
@maartenbreddels
Copy link
Contributor

Nice, I didn't expect it to be as simple, LGTM, also #309

@codecov
Copy link

codecov bot commented Apr 19, 2022

Codecov Report

Merging #2292 (abae5c6) into main (18f6ea9) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2292      +/-   ##
==========================================
+ Coverage   88.12%   88.14%   +0.01%     
==========================================
  Files         247      247              
  Lines       23309    23312       +3     
==========================================
+ Hits        20541    20548       +7     
+ Misses       2768     2764       -4     
Impacted Files Coverage Δ
glue/viewers/profile/layer_artist.py 94.56% <100.00%> (+0.12%) ⬆️
glue/viewers/profile/state.py 99.02% <100.00%> (+<0.01%) ⬆️
glue/conftest.py 67.85% <0.00%> (+4.76%) ⬆️

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 18f6ea9...abae5c6. Read the comment docs.

@astrofrog
Copy link
Member

As mentioned on Slack, I don't think we want to make these changes here - the Matplotlib viewers already draw things as steps by default so it is really the bqplot layer artist in glue-jupyter which should be updated to include this kind of logic.

If we do want to support the option of having both draw styles, then I do support adding a drawstyle or as_steps option to the state, but it should then default to True and simply toggle the drawstyle in the Matplotlib viewer, and again the logic for glue-jupyter should live specifically in the bqplot layer artist (since the Matplotlib layer artist will be taken from glue-core and already draw as steps).

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! Just a small comment below about removing unused code.

@@ -238,6 +240,7 @@ def __init__(self, layer=None, viewer_state=None, **kwargs):

self.add_callback('layer', self._on_layer_change, priority=1000)
self.add_callback('visible', self.reset_cache, priority=1000)
#self.add_callback('as_steps', self._on_as_steps_change, priority=1000)
Copy link
Member

Choose a reason for hiding this comment

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

Remove if unused?

@astrofrog
Copy link
Member

Can you also fix the codestyle failure (run tox -e codestyle locally to check) and add a changlog entry?

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.

3 participants