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

DM-23104: Enable per-band flags in transform object #351

Merged
merged 2 commits into from Feb 15, 2020

Conversation

yalsayyad
Copy link
Contributor

No description provided.

@yalsayyad yalsayyad changed the title DM-23104 DM-23104: Enable per-band flags Feb 14, 2020
@yalsayyad yalsayyad changed the title DM-23104: Enable per-band flags DM-23104: Enable per-band flags in transform object Feb 14, 2020
Copy link
Contributor

@morriscb morriscb left a comment

Choose a reason for hiding this comment

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

Looks good, explaining the slight change in Functor default behavior would be good though.

_radians = True
_defaultNoDup = True
_defaultNoDup = False
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for this change in the default behavior?

Copy link
Contributor Author

@yalsayyad yalsayyad Feb 14, 2020

Choose a reason for hiding this comment

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

👍 I tried to explain in the commit message, but it could use clarifying. Lots of words here:

We have a CoordColumn which is more specific than Column (it can convert radians to deg) and RA|DecColumn which is a special case of CoordColumn that looks for columns coord_ra and coord_dec. Before this ticket, CoordColumn doesn't explode out per band. Why not? I didn't want to write separate non-expanding CoordColumn because we can have those already with RAColumn and DecColumn. The alternative, (exploding) Column <-- ExplodingCoordColumn <-- (non-exploding) CoordColumn <-- (non-exploding)[RA|Dec]Column for functors that are all very slight variations of each other seems like overkill vs. (exploding) Column <-- (exploding) CoordColumn <-- (non-exploding)[RA|Dec]Column which is what this PR proposes. I think the only person who would notice is @timothydmorton

by splitting the `flags` key into two:
* `flags` which will expanded out per band from meas
* `refFlags` will be taked from ref
by behaving like other Columns which explode per band by default.
RAColumn and DecColumn are now the non-exploding special cases
that still produce a single reference band column.
Change the behavior of intermediate CoordColumn preferred over
adding a second intermediate class in the class hierarchy.
@yalsayyad yalsayyad merged commit 3e2e8b6 into master Feb 15, 2020
@timj timj deleted the tickets/DM-23104 branch February 18, 2021 15:49
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