-
Notifications
You must be signed in to change notification settings - Fork 9
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
Adding support for subsets in time #12
Conversation
24bd170
to
ca2aa1c
Compare
The failing test is coming from the |
Upstream fixes to jdaviz have been merged (spacetelescope/jdaviz#2168), will be released in 3.4.1. |
def _expected_subset_layer_default(self, layer_state): | ||
super()._expected_subset_layer_default(layer_state) | ||
|
||
layer_state.linewidth = 3 |
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.
can we add this is a lambda-inheritance (invented a new term) of SpecvizProfileView
instead?
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.
We could, but should we? It'll be clearer what it's doing this way.
def add_data(self, data, color=None, alpha=None, **layer_state): | ||
""" | ||
Overrides the base class to add markers for plotting | ||
uncertainties and data quality flags. | ||
|
||
Parameters | ||
---------- | ||
data : :class:`glue.core.data.Data` | ||
Data object with the spectrum. | ||
color : obj | ||
Color value for plotting. | ||
alpha : float | ||
Alpha value for plotting. | ||
|
||
Returns | ||
------- | ||
result : bool | ||
`True` if successful, `False` otherwise. | ||
""" | ||
# The base class handles the plotting of the main | ||
# trace representing the spectrum itself. | ||
result = super().add_data(data, color, alpha, **layer_state) | ||
|
||
# Set default linewidth on any created spectral subset layers | ||
# NOTE: this logic will need updating if we add support for multiple cubes as this assumes | ||
# that new data entries (from model fitting or gaussian smooth, etc) will only be spectra | ||
# and all subsets affected will be spectral | ||
for layer in self.state.layers: | ||
if "Subset" in layer.layer.label and layer.layer.data.label == data.label: | ||
layer.linewidth = 3 | ||
|
||
return result |
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.
can this also use lambda-inheritance of SpecvizProfileView.add_data
? The only difference seems to be that that includes calls to self._plot_uncertainties()
and self._plot_mask()
, but we can have those both just pass here and create follow-up issues to fix those implementations (either custom implementations in lcviz or generalize upstream so they can be inherited).
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.
We can indeed. I think this is more readable than sending the reader to read upstream, but your call 😃
63ef43d
to
cdf83a6
Compare
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.
Upstream fixes to jdaviz have been merged (spacetelescope/jdaviz#2168), will be released in 3.4.1.
If the tests pass for you locally, then I'd be ok with bumping the pin here to 3.4.1 (so the tests fail saying there is no 3.4.1 until there actually is, rather than other failures caused by 3.4.0 and then later having to remember to bump the pin), and merging.
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 to see subsets working again. Thanks Brett!
cdf83a6
to
7e12c8f
Compare
Merging despite test failures. We'll need to update the jdaviz pin to 3.5 after that gets released to pick up the changes in spacetelescope/jdaviz#2168 which support this PR. |
This PR gets time subsets working for LCviz. 🐱
lcviz_subset_fix.mov
Covered in this PR:
LightCurve
s (will later useful for masking)LightCurve
translator in lcviz to enable subset selections from light curvesCoordinates
subclass to use for the time axis (analog of glue-astro’sSpectralCoordinates
)KeplerLightCurve
andTessLightCurve
subclasses ofLightCurve
(so the type of the input*LightCurve
object is preserved on round-trip)This PR depends on upstream updates to jdaviz as of spacetelescope/jdaviz#2168, which adds support for temporal subset selection.