-
Notifications
You must be signed in to change notification settings - Fork 10
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
TPF viewer #81
TPF viewer #81
Conversation
565583a
to
2e23a90
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature-tpf #81 +/- ##
==============================================
Coverage ? 93.36%
==============================================
Files ? 35
Lines ? 1734
Branches ? 0
==============================================
Hits ? 1619
Misses ? 115
Partials ? 0 ☔ View full report in Codecov by Sentry. |
# Hide axes by default | ||
self.state.show_axes = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just commenting that I like show_axes = False
as the default. I see you toggle axes back on in the example code – handy to have the option, but takes up a lot of space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, yes, that was mostly for debugging and confirming the mouseover information, I don't think we'd want that in any public-facing example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! I've put some suggestions below, which we can copy/paste into tickets if you like.
Expose slice number somewhere in the image viewer
I know we will implement the slice plugin in a different PR, but I wanted to propose (somehow) that we annotate the image viewer with the slice number/time/cadence number. If/when we export the image in the image viewer to PNG, for example, it's only useful if you know which slice it came from. lightkurve gives you the TPF slice plots with a cadence number label in the title, but I'm not sure where we'd put it.
You could argue that an annotation is redundant if you can see the slice marker in the light curve viewer or if you have the slice plugin open. But if you're zoomed elsewhere in the LC viewer, or don't have the slice plugin open, that info isn't shown.
If we don't want to put the annotation over the data in the viewer somewhere, one compromise could be in mouseover info for the image viewer.
Image viewer WCS
I confirmed that in your example, tpf.wcs
returns WCS for the image in the image viewer. It might be straightforward to use this WCS in the cube viewer (with padding for the time axis for glue via PaddedTimeWCS
, already implemented in this branch).
# TODO: move this to an event listener on add_data so that we can also remove when empty? | ||
from jdaviz.core.events import NewViewerMessage | ||
from lcviz.viewers import CubeView | ||
viewer_reference_name = 'image' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good name. 😊
lcviz/viewers.py
Outdated
# TODO: can we vary this default_class based on Kepler vs TESS, etc? | ||
default_class = KeplerTargetPixelFile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious to check in on this. Does the mission specificity matter in the viewer class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I wasn't sure what to do with this 🤔 . Ultimately this is just for the default and the user can override, but maybe its better to just have as None and require the user to pass a class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done and linked a comment back here if we ever want to revisit.
['jdaviz:homezoom', 'jdaviz:prevzoom'], | ||
['jdaviz:boxzoom'], | ||
['jdaviz:panzoom'], | ||
['bqplot:rectangle'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything stopping us from allowing other ROI shapes at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really, but we'll have to see what we want to support for actual photometric extraction (and if we want sub-pixel support or not).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but even rectangle ROIs aren't required to have 100% overlap with the pixel grid. I don't see a difference with other shapes in that sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, we might decide to just do pixel-by-pixel masking if we don't want sub-pixel at all. Let's consider this when we write the extraction plugin - as of now the subsets don't do anything but I just wanted at least something to show up in the menu to test that they are created succesfully.
I agree this would be useful in some way, but think we might want to consider this as a general feature upstream in cubeviz? |
* 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 <[email protected]>
This PR implements a dynamically-created TPF viewer.
might need logic or refactoring upstream- see lcviz cube support jdaviz#2678)Note that in order to get correct mouseover and stretch histogram, spacetelescope/jdaviz#2678 is required (which is targeted for jdaviz 3.9). Even with that, the default stretch percentile often drives the whole range, requiring using a custom stretch 🐱.
Example case:
Manually setting stretch and colormap:
Until the slice plugin is integrated, slicing can be done manually:
Next up: