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

add specviz property to cubeviz/mosviz helpers #330

Merged
merged 3 commits into from
Apr 12, 2021

Conversation

eteq
Copy link
Contributor

@eteq eteq commented Oct 8, 2020

This implements the feature I suggested in #329. This PR should probably also include some doc updates showing its use a la what @PatrickOgle originally was thinking about in #257 (comment). The idea is that with this you can do something like:

cubeviz = CubeViz()
cubeviz.load_data(...)

cubeviz.specviz.some_specviz_helper(...)

and that should be documented as the intended approach to using specviz helpers in the cubeviz context. But I'm not doing that now because I want to make sure others are ok with this idea before I complete this. Hence the "draft" status.

@eteq eteq marked this pull request as draft October 8, 2020 17:33
@codecov
Copy link

codecov bot commented Oct 8, 2020

Codecov Report

Merging #330 (aeeb498) into main (5a7f571) will increase coverage by 1.01%.
The diff coverage is 60.00%.

❗ Current head aeeb498 differs from pull request most recent head 0f0c8ab. Consider uploading reports for the commit 0f0c8ab to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #330      +/-   ##
==========================================
+ Coverage   65.04%   66.05%   +1.01%     
==========================================
  Files          49       47       -2     
  Lines        2818     2740      -78     
==========================================
- Hits         1833     1810      -23     
+ Misses        985      930      -55     
Impacted Files Coverage Δ
jdaviz/configs/cubeviz/helper.py 60.00% <57.14%> (-6.67%) ⬇️
jdaviz/configs/mosviz/helper.py 70.32% <62.50%> (-1.44%) ⬇️
jdaviz/core/data_formats.py
jdaviz/core/config.py
jdaviz/app.py 83.19% <0.00%> (+3.03%) ⬆️
jdaviz/core/events.py 75.48% <0.00%> (+4.51%) ⬆️
jdaviz/configs/mosviz/plugins/viewers.py 87.75% <0.00%> (+5.57%) ⬆️

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 4ead05b...0f0c8ab. Read the comment docs.

@eteq
Copy link
Contributor Author

eteq commented Nov 2, 2020

After some out-of-band discussion w/ @javerbukh @dtn5ah @rosteen @havok2063 , there's some agreement that this is the way we want to go as an idiom for connecting together "sub-vizes". So I'm converting to "regular" PR.

@eteq eteq marked this pull request as ready for review November 2, 2020 19:53
Base automatically changed from master to main January 29, 2021 19:45
Copy link
Collaborator

@rosteen rosteen left a comment

Choose a reason for hiding this comment

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

We definitely should make a note of this in the docs, and probably add a test for Mosviz and Cubeviz to check that it works (maybe just a call to e.g. viz.specviz.get_spectra() or something). Do you have bandwidth to make those additions @eteq or should someone else take over?

Edit: I should also note that I tested the actual functionality in Cubeviz and Mosviz and it works in both.

@eteq
Copy link
Contributor Author

eteq commented Feb 5, 2021

Hmm. I think I have time, but I have said that before and been wrong. Maybe give it another several days, @rosteen, and if I haven't gotten to it by then someone else should jump in?

Copy link
Contributor

@ibusko ibusko left a comment

Choose a reason for hiding this comment

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

I tested the basic loading sequence and it works. LGTM

Copy link
Collaborator

@rosteen rosteen left a comment

Choose a reason for hiding this comment

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

Approving this, I have a couple docs additions that I'll put in a follow-up PR shortly, rather than adding them here. I'll resolve the merge conflicts here.

rosteen added a commit to rosteen/jdaviz that referenced this pull request Apr 12, 2021
Add specviz property to cubeviz/mosviz helpers
@eteq eteq force-pushed the specviz-helper-helper branch from aeeb498 to eba661c Compare April 12, 2021 14:43
Copy link
Collaborator

@rosteen rosteen left a comment

Choose a reason for hiding this comment

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

Suggested two blank line addition/deletions to fix codestyle errors.

jdaviz/configs/cubeviz/helper.py Show resolved Hide resolved
jdaviz/configs/mosviz/helper.py Show resolved Hide resolved
@rosteen rosteen merged commit ba65f9d into spacetelescope:main Apr 12, 2021
@eteq eteq deleted the specviz-helper-helper branch April 12, 2021 15:28
rosteen added a commit to rosteen/jdaviz that referenced this pull request Apr 12, 2021
Add specviz property to cubeviz/mosviz helpers
# 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