Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

as_steps support in profile viewer #309

Merged
merged 7 commits into from
Aug 26, 2022

Conversation

kecnry
Copy link
Contributor

@kecnry kecnry commented Apr 19, 2022

Description

This accompanies glue-viz/glue#2292 to expose the as_steps switch in the layer options widget, and handling toggling as_steps in the bqplot layer artist for the profile viewer. The actual binning logic is adopted from https://github.com/astropy/specutils/blob/main/specutils/spectra/spectral_axis.py#L42.

@kecnry kecnry changed the title Profile viewer as steps as_steps support in profile viewer Apr 19, 2022
@kecnry kecnry marked this pull request as ready for review April 19, 2022 15:17
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.

As mentioned in glue-viz/glue#2292 (comment), I think we should instead be implementing the logic of converting the profile into a stepped profile in the bqplot layer artist so that it would be analogous to the fact that this conversion is done in the layer artist for Matplotlib (in fact it is done inside Matplotlib because of the drawstype='steps' option).

@@ -6,5 +6,8 @@
<div>
<v-select label="attribute" :items="attribute_items" v-model="attribute_selected" hide-details />
</div>
<div>
<v-switch label="Plot as steps/histogram" v-model="as_steps" />
Copy link
Member

Choose a reason for hiding this comment

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

I would avoid using the terminology 'histogram' here, maybe just use '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 and works great! Can you add a changelog entry? Also we will need to wait to merge this until a new version of glue-core is released, will try and do shortly once we merge the other PR.

@kecnry
Copy link
Contributor Author

kecnry commented Apr 22, 2022

Just noticed that this seems to break when adding a subset (within the bqplot steps logic)... we'll need to fix that before merging/releasing.

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.

Could you check that this works with glue-core 1.3.0 and if so bump the minimum glue-core version in setup.cfg? Once that is done I think we can merge this.

@codecov
Copy link

codecov bot commented May 19, 2022

Codecov Report

Merging #309 (3cd9d31) into main (f31402d) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #309      +/-   ##
==========================================
+ Coverage   89.14%   89.16%   +0.02%     
==========================================
  Files          86       86              
  Lines        4275     4284       +9     
==========================================
+ Hits         3811     3820       +9     
  Misses        464      464              
Impacted Files Coverage Δ
glue_jupyter/bqplot/profile/layer_artist.py 93.06% <100.00%> (+0.51%) ⬆️
glue_jupyter/common/state_widgets/layer_profile.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@kecnry kecnry force-pushed the profile-viewer-as-steps branch from 60958e6 to 3cd9d31 Compare August 24, 2022 16:51
@kecnry
Copy link
Contributor Author

kecnry commented Aug 24, 2022

Sorry this fell off the radar for a little while. I rebased, confirmed this works with glue 1.3+, and set that as a requirement in setup.cfg.

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 great, thanks!

@dhomeier dhomeier merged commit c7853e1 into glue-viz:main Aug 26, 2022
@kecnry kecnry deleted the profile-viewer-as-steps branch August 26, 2022 17:19
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants