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

Time Selector (adapted version of cubeviz's slice) plugin #85

Merged
merged 9 commits into from
Mar 11, 2024

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Feb 14, 2024

This PR implements a modified version of cubeviz's slice plugin into lcviz for TPF support.

NOTE: this PR requires the following upstream PRs and therefore an update to the jdaviz pin, and until then the RTD build and tests are expected to fail:

Now that these are all accepted and merged into jdaviz, this PR should be tested against jdaviz main (3.9).

And the following upstream draft PR was created for this, but is no longer used within this PR, so is not necessary:

This will then also bump #82 to require the necessary pin of jdaviz (likely 3.9).

Example usage:

from lightkurve import search_lightcurve, search_targetpixelfile
from lcviz import LCviz

lc = search_lightcurve("HAT-P-11", mission="Kepler", cadence="long", quarter=10).download() #.flatten()
tpf = search_targetpixelfile("HAT-P-11", mission="Kepler", cadence="long", quarter=10).download()


lcviz = LCviz()
lcviz.load_data(lc)
lcviz.load_data(tpf)

po = lcviz.plugins['Plot Options']
po.viewer = 'image'
po.image_colormap = 'Gray'
po.stretch_function = 'Arcsinh'
po.stretch_vmin = 0
po.stretch_vmax = 20000

lcviz.show()
Screen.Recording.2024-03-04.at.12.17.29.PM.mov
Screen.Recording.2024-03-06.at.3.24.34.PM.mov

@kecnry kecnry force-pushed the tpf-slice branch 2 times, most recently from 556ce65 to 016f5ea Compare February 14, 2024 18:26
@kecnry kecnry mentioned this pull request Feb 15, 2024
1 task
@kecnry kecnry force-pushed the tpf-slice branch 3 times, most recently from 2bd1979 to 14d8a7a Compare February 16, 2024 21:03
* still requires a notebook hack to send the actual data

from astropy import units as u
times = lcviz.viewers['image']._obj.data()[0].get_component('dt').data[:, 0, 0]
sl._obj._update_data(times * u.d)
* slice plugin is hidden if no cube-like data
* requires upstream support, subject to change
@kecnry kecnry force-pushed the tpf-slice branch 2 times, most recently from 8676494 to c92ea66 Compare February 19, 2024 19:25
@kecnry kecnry changed the title implement basic (not yet functional) slice plugin Time Selector (slice) plugin Feb 19, 2024
* removes need for context-aware as the plugin is now always relevant with synced time indicators
* renames "Slice" plugin to "Time Selector"
@kecnry kecnry requested a review from bmorris3 March 4, 2024 17:18
@kecnry kecnry changed the title Time Selector (slice) plugin Time Selector (adapted version of cubeviz's slice) plugin Mar 4, 2024
@bmorris3
Copy link
Contributor

bmorris3 commented Mar 4, 2024

It looks like the default selected tool is slice, even for light curves without TPFs. If you press Home, it keeps slice activated:

Screen Shot 2024-03-04 at 14 23 54

Is that intentional?

Copy link
Contributor

@bmorris3 bmorris3 left a comment

Choose a reason for hiding this comment

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

If the loaded data is a TPF, we agreed that the time selector should snap to slice, which looks good.

I load a light curve (not a TPF) into the demo notebook, the slicing logic doesn't appear to be applied. When I hover on an in-transit cadence in flux-vs-time until the correct cadence is highlighted, and click it with the Slice tool active, I never get an in-transit slice in the ephemeris viewer:

lcviz-slice-snap.mov

I assume this is because my mouse clicks are >1 transit duration from the center of the in-transit exposure, but that's hard to tell.

Let's ensure that snap to slice is active for LCs.

@kecnry
Copy link
Member Author

kecnry commented Mar 4, 2024

I think what you're describing (if I followed it correctly) is a little more subtle (and quite a bit more complicated) than snapping vs not snapping. There are a few coupled things going on here:

  1. snap-to-slice currently means snap to the slice in a cube, not a data point in the viewer with the indicator. Snapping is enabled here by default, but when there is no cube, then it is effectively ignored. I don't think we want to change this to "snap to data point in the indicator viewer", because then that can result in an annoying experience when a cube is loaded but sampled at different times than the light curve. The main point of snapping is so that the indicator exactly matches the image shown. We could have a separate option for "snap to point"... or when there is no cube loaded, snap-to-slice could then actually snap to data points (that feels inconsistent to me though and might be confusing logic).
  2. even if you had a cube loaded, I don't think it would behave as you're expecting because the slice tool works only considers the slice (x) axis, whereas the mouseover display is doing nearest on the screen (in xy). You'll notice in your video that the indicator did move to your cursor, but not the orange rectangle. So should the mousover behavior change so that it feels more consistent with the slice tool? Or does this different behavior make sense because they're different tools? And if we want to make any changes, should those be isolated to lcviz or be made upstream?

@kecnry
Copy link
Member Author

kecnry commented Mar 4, 2024

It looks like the default selected tool is slice, even for light curves without TPFs. If you press Home, it keeps slice activated. Is that intentional?

Yes, I couldn't see any reason for it not to be the default since there currently was no other default. Do you disagree with that?

@kecnry
Copy link
Member Author

kecnry commented Mar 6, 2024

This now supports dragging the indicator in the phase-viewer (by computing a delta time from a delta phase and setting the time in the same cycle).

Screen.Recording.2024-03-06.at.3.24.34.PM.mov

@kecnry kecnry marked this pull request as ready for review March 8, 2024 20:09
@kecnry
Copy link
Member Author

kecnry commented Mar 11, 2024

Ok, we're merging this as is into the feature branch and then will consider the points above as we flesh out the rest of the functionality. Thanks for the feedback!

@kecnry kecnry merged commit a81dea6 into spacetelescope:feature-tpf Mar 11, 2024
1 of 9 checks passed
@kecnry kecnry deleted the tpf-slice branch March 11, 2024 14:43
kecnry added a commit that referenced this pull request Apr 5, 2024
* Adding TPF translator and parser (#75)
* TPF viewer (#81)
* make use of upstream refactor to override indices in lcviz (#83)
* fix creating phase-viewer when TPF is loaded (#86)
* Time Selector (adapted version of cubeviz's slice) plugin (#85)
* enable clone viewer for image/TPF viewer (#101)

---------

Co-authored-by: Brett M. Morris <bmmorris@stsci.edu>
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