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

features.util.bounded_is_fixed() does not work for dust_feature-only science packs #274

Closed
drvdputt opened this issue Jan 30, 2024 · 5 comments

Comments

@drvdputt
Copy link
Contributor

When a science pack is created that only has dust_feature entries, there will be no masked values in the power column of the Features table. As a consequence, the astropy table constructor automatically decides whether to use a Column instead of a MaskedColumn. This means that any functionality that relies on the presence of a mask attribute, wil fail.

E.g. in featuresutil.bounded_is_fixed(): return ma.getmask(val)[..., -2:].all(-1), where getmask will return False.

Most consistent solution: Force the use of MaskedColumn (or in our case BoundedMaskedColumn) when the Features table.

Even if we found another way around this bug, I think we should enforce this to ensure consistency. (e.g. What if the user want to set power boundaries, but the power column does not support masks because of the way the table was loaded?)

@jdtsmith
Copy link
Contributor

jdtsmith commented May 8, 2024

I agree we can just enforce all columns are BoundedMaskedColumns even if most of the elements are "fixed".

@jdtsmith
Copy link
Contributor

jdtsmith commented May 9, 2024

OK, update, never mind that. #283 includes a fix for this. It should create bounds for all parameter columns except for string-parameters (name, group, model, geometry). The latter should never be passed to the bounds checker functions.

@jdtsmith
Copy link
Contributor

Double check if this is fixed, please.

@drvdputt
Copy link
Contributor Author

drvdputt commented May 13, 2024

Yes, seems to work now, since BoundedMaskedColumn doesn't exist anymore, this bug can also no longer appear. Everything is just a regular Column, even in this dust_feature-only edge case.

@jdtsmith
Copy link
Contributor

I didn't check all the utils convenience functions but we can revisit if needed.

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

No branches or pull requests

2 participants