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.

Sign up for GitHub

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

Jump to bottom

Flatten: separate previews for flattened and trend #41

Merged
merged 2 commits into from
Oct 19, 2023
Merged

Flatten: separate previews for flattened and trend #41

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f53454
separate toggle for flattened and trend live-preview visibility
kecnry Oct 18, 2023
814c276
Apply suggestions from code review
kecnry Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions lcviz/plugins/flatten/flatten.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,10 @@ class Flatten(PluginTemplateMixin, DatasetSelectMixin, AddResultsMixin):
Only the following attributes and methods are available through the
public plugin API.

* ``show_live_preview``
* ``show_live_preview`` : bool
Whether to show the live-preview of the (unnormalized) flattened light curve
* ``show_trend_preview`` : bool
Whether to show the live-preview of the removed trend
kecnry marked this conversation as resolved.
Show resolved Hide resolved
* ``default_to_overwrite``
* ``dataset`` (:class:`~jdaviz.core.template_mixin.DatasetSelect`):
Dataset to flatten.
Expand All @@ -43,6 +46,7 @@ class Flatten(PluginTemplateMixin, DatasetSelectMixin, AddResultsMixin):
uses_active_status = Bool(True).tag(sync=True)

show_live_preview = Bool(True).tag(sync=True)
show_trend_preview = Bool(True).tag(sync=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
show_trend_preview = Bool(True).tag(sync=True)
show_trend_preview = Bool(False).tag(sync=True)

Take or leave this suggestion, but I think the default should be less cluttered and redundant. Especially since the trend color and live preview color are identical, there's a lot to see in the viewer if this is enabled by default.

Copy link
Member Author

Choose a reason for hiding this comment

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

usually I actually prefer to see the trend over the preview 🤷‍♂️ Let's leave for now and revisit as we use it more.

default_to_overwrite = Bool(True).tag(sync=True)
flatten_err = Unicode().tag(sync=True)

Expand All @@ -65,7 +69,7 @@ def __init__(self, *args, **kwargs):

@property
def user_api(self):
expose = ['show_live_preview', 'default_to_overwrite',
expose = ['show_live_preview', 'show_trend_preview', 'default_to_overwrite',
'dataset', 'add_results',
'window_length', 'polyorder', 'break_tolerance',
'niters', 'sigma', 'unnormalize', 'flatten']
Expand Down Expand Up @@ -160,17 +164,19 @@ def _clear_marks(self):
mark.clear()
mark.visible = False

@observe('is_active', 'show_live_preview')
@observe('is_active', 'show_live_preview', 'show_trend_preview')
def _toggle_marks(self, event={}):
visible = self.show_live_preview and self.is_active
live_visible = self.show_live_preview and self.is_active
trend_visible = self.show_trend_preview and self.is_active

trend_marks, flattened_marks = self.marks
for mark in trend_marks.values():
mark.visible = visible
mark.visible = trend_visible
for mark in flattened_marks.values():
mark.visible = visible
mark.visible = live_visible

if visible and event.get('name') in ('is_active', 'show_live_preview'):
if ((live_visible or trend_visible) and
event.get('name') in ('is_active', 'show_live_preview', 'show_trend_preview')):
# then the marks themselves need to be updated
self._live_update(event)

Expand All @@ -187,7 +193,7 @@ def _live_update(self, event={}):
return
self.flatten_err = ''

if event.get('name') not in ('is_active', 'show_live_preview'):
if event.get('name') not in ('is_active', 'show_live_preview', 'show_trend_preview'):
# mark visibility hasn't been handled yet
self._toggle_marks(event)

Expand Down
12 changes: 10 additions & 2 deletions lcviz/plugins/flatten/flatten.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,22 @@
<v-row>
<v-switch
v-model="show_live_preview"
label="Show live preview"
hint="Whether to show live preview of flattening options. Note that the live-preview of the flattened light curve is not yet normalized."
label="Show flattened preview"
hint="Whether to show live-preview of the unnormalized flattened light curve."
persistent-hint
></v-switch>
</v-row>
<v-row v-if="show_live_preview && !unnormalize">
<v-alert type="warning">Live preview is unnormalized, but flattening will normalize.</v-alert>
</v-row>
<v-row>
<v-switch
v-model="show_trend_preview"
label="Show trend preview"
hint="Whether to show live-preview of the trend used for flattening."
persistent-hint
></v-switch>
</v-row>
<v-row>
<v-switch
v-model="default_to_overwrite"
Expand Down