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

Flux Column plugin #77

Merged
merged 10 commits into from
Jan 10, 2024
Merged

Flux Column plugin #77

merged 10 commits into from
Jan 10, 2024

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Dec 28, 2023

This PR implements a plugin for selecting the flux origin per-dataset (note that this means that each dataset chooses which column is treated as flux, and then every viewer and plugin will use that column as input).

Screen.Recording.2023-12-28.at.3.28.18.PM.mov

This also results in some necessary changes to the flatten plugin - which no longer writes to a new data entry, but rather appends a new column in the input light curve and selects that as the flux-origin by default. This does make some workflows like comparing before and after flattening a little more cumbersome, but should be more natural for the more common use-cases:

Screen.Recording.2023-12-29.at.3.45.56.PM.mov

Rendered plugin docs: https://lcviz--77.org.readthedocs.build/en/77/plugins.html#flux-column

TODO:

  • outputs from flattening do not play well because: the flux column is unitless, tripping the unit comparison logic, and other columns remain unflattened. Would we want an option in flattening to append as a new column to the existing dataset instead of overwriting? Should flattening plugin apply across all flux columns or drop remaining columns?
  • test workflow with binning. Binning does bin each column (and has a different time column), so creating a new data entry here does make sense and works as-is.
  • clear cache on ANY dataset dropdown when flux column is changed (test with binning live-preview)

Potential follow-up:

  • update or hide FLUX_ORIGIN from metadata plugin
  • what if the user wants to plot these columns side-by-side (either in the same or separate viewers)? Maybe a "clone" data (similar to clone viewer) would be appropriate in that case?

@kecnry kecnry requested a review from bmorris3 December 28, 2023 20:32
Copy link

codecov bot commented Dec 28, 2023

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (a4eaea7) 95.04% compared to head (d7882a8) 94.49%.
Report is 1 commits behind head on main.

Files Patch % Lines
lcviz/components/components.py 92.64% 5 Missing ⚠️
lcviz/plugins/flatten/flatten.py 81.48% 5 Missing ⚠️
lcviz/tools.py 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #77      +/-   ##
==========================================
- Coverage   95.04%   94.49%   -0.55%     
==========================================
  Files          32       36       +4     
  Lines        1393     1545     +152     
==========================================
+ Hits         1324     1460     +136     
- Misses         69       85      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kecnry kecnry marked this pull request as ready for review January 2, 2024 13:32
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.

Sorry for this small but annoying-to-address comment/question.

docs/plugins.rst Outdated
@@ -35,6 +35,41 @@ This plugin allows viewing of any metadata associated with the selected data.
:ref:`Jdaviz Metadata Viewer <jdaviz:imviz_metadata-viewer>`
Jdaviz documentation on the Metadata Viewer plugin.

.. _flux-origin:

Flux Origin
Copy link
Contributor

Choose a reason for hiding this comment

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

I know what you mean by origin but the term used throughout lightkurve is column. Is there a reason to introduce another term?

Copy link
Member Author

@kecnry kecnry Jan 5, 2024

Choose a reason for hiding this comment

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

hmmm I actually originally had column, but then changed it because its exposed as "FLUX_COLUMN" in the metadata and the string representation of the LightCurve object 🤔

I guess this and this both use "column", but this tutorial shows how many times "origin" also appears.

Ultimately it's not too difficult to change (now or before this makes a release - significantly more difficult after a release)... so we can either decide which one we think is better or exposed more publicly or ask if they'd ever consider moving to be more self-consistent within lightkurve and go with that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Personally, I'd reserve "origin" for the origin of the axis, which might be, e.g., zero or one, depending on units and normalization.

@kecnry kecnry changed the title Flux origin plugin Flux Column plugin Jan 5, 2024
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.

I read everything and tested it and it works great, thanks! The comment I left doesn't block merge.

:items="flux_column_items.map(i => i.label)"
v-model="flux_column_selected"
label="Flux Column"
hint="Select the column to adopt as flux."
Copy link
Contributor

Choose a reason for hiding this comment

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

I would love to put a link here to someone's docs that define the columns, so I looked around online to see if the Kepler/TESS/NASA docs have a good description of the column name definitions, but none are obvious. The relevant bit of the lightkurve docs don't link elsewhere. Eventually, let's have a brief expansion of the acronyms, and a literature reference for Kepler+TESS column definitions in our docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

what would be awesome is if we could just pull descriptions from somewhere on-the-fly for the currently selected column, or at least change the link based on the "author" (need to think about the HLSP case here as well).

@kecnry kecnry merged commit c7776c4 into spacetelescope:main Jan 10, 2024
9 of 11 checks passed
@kecnry kecnry deleted the plg-flux-column branch January 10, 2024 13:41
kecnry added a commit to bmorris3/lcviz that referenced this pull request Jan 19, 2024
* spacetelescope#77 uses .columns which is only supported by light curves, not TPFs
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