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

Revert column to be Quantity rather than MaskedQuantity for Kepler/TESS LCs, i.e., pre v2.1 / Astropy5 behavior. #1281

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

orionlee
Copy link
Collaborator

@orionlee orionlee commented Feb 12, 2023

This PR is to make Quantity-like columns for TESS / Kepler lightcurves, e.g., flux, sap_flux, etc. to be Quantity, rather than MaskedQuantity.

It essentially reverts to the behavior of Lightkurve 2.0.x with Astropy 4.x.
The reason is to avoid various complication with MaskedQuantity, most recently errors involving astropy sigma_clip() with numpy 1.24 .

If this PR is merged, it does not negate the recent commits and PR #1280 on the workarounds in calling sigma_clip(). The PR would just make them less urgent, as MaskedQuantity will no longer be in common use cases. (Users can still explicitly use MaskedQuantity for whatever reasons.)

…py5 behavior

- to avoid complications with MaskedQuantity
@orionlee orionlee changed the title Revert column to be Quantity for Kepler/TESS LCs, pre lk v2.1 / Astropy5 behavior Revert column to be Quantity for Kepler/TESS LCs, i.e., pre v2.1 / Astropy5 behavior. Feb 12, 2023
@christinahedges
Copy link
Collaborator

Hi @orionlee thanks for bringing this up! In this PR, if a user were to input mask data e.g. because of a flare or a cosmic ray, this data would be over-written with a np.nan value. Users may find that this isn't what they want, e.g. they may want a data value temporarily masked for some analysis.

What do you think?

@orionlee
Copy link
Collaborator Author

orionlee commented Feb 13, 2023

@christinahedges Could you elaborate on the temporary mask use case you mentioned?

In my mind, the existing boolean indexing works just fine without using MaskedQuantity:

mask = some_function_to_define_cosmic_ray_mask(lc)
some_analysis_function(lc[~mask])  # temporary mask out cadences with cosmic ray

Using MaskedQuantity is arguably more cumbersome, e.g.,

# temporarily mask out flux at index 123
lc_tmp = lc.copy()
lc_tmp.flux.mask[123] = True
# if sap_flux is also used by the analysis, need to mask it too
lc_tmp.sap_flux.mask[123] = True
some_analysis_function(lc_tmp)

The change in this PR is only concerned with reading TESS / Kepler FITS file.

  • The current behavior is to replace nan values with mask.
  • This PR exposes the underlying nan values (lightkurve 2.0.x behavior)

It has nothing direct bearing on user temporarily masking out specific cadence / values, unless users assume columns are MaskedQuantity and use Masked-specific API such as .mask .


Irrespective on the merits of using MaskedQuantity

  1. This PR does not stop users from using MaskedQuantity, but it is no longer the de-facto column type for TESS / Kepler LCs. If users have specific needs to use MaskedQuantity, they can still explicitly replace a column (of type Quantity) with the MaskedQuantity counterpart.
  2. If it's decided that we do want to retain MaskedQuantity as the column type for TESS / Kepler LCs, I think we should:
    a. document it somewhere
    b. ensure we do always return MaskedQuantity. The current behavior is dependent on the contents of the specific FITS file: if a a column contains nan, the column will be MaskedQuantity, otherwise, it'd be regular Quantity.
    In other words, lightkurve does not guarantee a column would be MaskedQuantity. Users cannot exactly assume it to be the case. MaskedQuantity becomes de-facto type for Kepler/TESS LCs because in most cases, columns have nan values.

@christinahedges
Copy link
Collaborator

Hi @orionlee thanks for the explanation, when you say

The current behavior is to replace nan values with mask.

what do you mean here?

There is also a comment in the astropy Kepler file reader that this is a workaround they want to get rid of when masked quantities are supported better. Given that, do you think this is still a good fix to implement in lightkurve?

@orionlee orionlee changed the title Revert column to be Quantity for Kepler/TESS LCs, i.e., pre v2.1 / Astropy5 behavior. Revert column to be Quantity rather than MaskedQuantity for Kepler/TESS LCs, i.e., pre v2.1 / Astropy5 behavior. Nov 4, 2023
@orionlee orionlee changed the title Revert column to be Quantity rather than MaskedQuantity for Kepler/TESS LCs, i.e., pre v2.1 / Astropy5 behavior. Revert column to be Quantity rather than MaskedQuantity for Kepler/TESS LCs, i.e., pre v2.1 / Astropy5 behavior. Nov 4, 2023
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.

None yet

2 participants