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

Use upstream updates to plugin plots #55

Merged
merged 6 commits into from
Nov 30, 2023

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Oct 19, 2023

This updates the plot in frequency analysis to account for changes upstream in spacetelescope/jdaviz#2498 (which adds tool and pan/zoom support), and in doing so requires jdaviz 3.8.

Note that this also means that without this PR, the former implementation may be broken once jdaviz 3.8 is released.

Waiting for: jdaviz 3.8

@kecnry kecnry added the waiting-on-jdaviz-release PR requires a release from jdaviz before updating the pin label Oct 19, 2023
@kecnry kecnry force-pushed the upstream-plugin-plots-to-viewer branch from ac76c8c to d03f949 Compare November 30, 2023 16:15
wasn't failing before because jdaviz didn't enforce not setting non-exposed attributes
@kecnry kecnry force-pushed the upstream-plugin-plots-to-viewer branch from 90d2d51 to c19297b Compare November 30, 2023 17:04
@kecnry kecnry marked this pull request as ready for review November 30, 2023 17:07
@kecnry kecnry requested a review from bmorris3 November 30, 2023 17:08
@kecnry kecnry removed the waiting-on-jdaviz-release PR requires a release from jdaviz before updating the pin label Nov 30, 2023
self.plot.figure.axes[1].label = 'power'
self.plot.figure.fig_margin = {'top': 60, 'bottom': 60, 'left': 65, 'right': 15}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should left be larger?

Screen Shot 2023-11-30 at 12 13 26

Copy link
Contributor

Choose a reason for hiding this comment

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

Correction: the line above is not responsible for the figure/viewer axis label spacing.

Copy link
Member Author

Choose a reason for hiding this comment

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

let's address the main viewer padding separately. It may be hard to get right in general, especially with such a large possible range in flux values.

@kecnry kecnry merged commit f3ab6f2 into spacetelescope:main Nov 30, 2023
6 of 7 checks passed
@kecnry kecnry deleted the upstream-plugin-plots-to-viewer branch November 30, 2023 17:21
bmorris3 pushed a commit to bmorris3/lcviz that referenced this pull request Dec 8, 2023
* update freq analysis plot to use glue-viewers
* improve axes styling/padding (in plugin plots)
* update jdaviz pin to 3.8
* update tests
* add binning.show_live_preview to user-api (wasn't failing before because jdaviz didn't enforce not setting non-exposed attributes)
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