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

WIP: Change LDOs to use masked products by default #661

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

e-koch
Copy link
Contributor

@e-koch e-koch commented Sep 2, 2020

This supersedes the first attempts in #540.

Our issue stems from subclassing u.Quantity for all LDOs (Projection, Slice, OneDSpectrum) while trying to use the masking routines built for spectral-cube (LDO.filled_data[:], LDO._get_filled_data(np.NaN)).

Basically LDO.filled_data[:] should be used by default for LDO[slice], LDO.value, LDO.quantity, etc. as this matches the behaviour used for SpectralCube. But this interferes with how we currently have u.Quantity subclassed.

@e-koch
Copy link
Contributor Author

e-koch commented Sep 2, 2020

@keflavich @astrofrog help would be appreciated here!

This may require disentangling u.Quantity as a subclass for LDOs, closer to what we use for cubes. A lot of the LDO code is already confusing/hacked together.

The current changes fix the primary issue of returning the data filled from the mask but fall apart when slicing an LDO as __new__ doesn't get called and most attributes don't get set.

@coveralls
Copy link

Coverage Status

Coverage decreased (-86.0%) to 0.0% when pulling 920df34 on e-koch:fix_projection_fill_value into 1497c07 on radio-astro-tools:master.

@keflavich
Copy link
Contributor

Questions from telecon:

  • Can we remove Quantity as a parent class? What capabilities would we need to add to a base class / mixin class to minimize breakage?
  • Alternatively, can we override .value to use the spectral-cube masking scheme?

If you try any of these approaches, keep notes here on what breaks.

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.

3 participants