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

Tickets/dm 7221 #85

Merged
merged 4 commits into from Aug 24, 2016
Merged

Tickets/dm 7221 #85

merged 4 commits into from Aug 24, 2016

Conversation

parejkoj
Copy link
Contributor

No description provided.

@timj
Copy link
Member

timj commented Aug 23, 2016

Is this going to cause difficulties for #84's merge?

// spellings of radians. We do continue to support no units for backwards
// compatibility.
if (!item.tunit.empty() && item.tunit != "rad") {
throw LSST_EXCEPT(afw::fits::FitsError, "Angle fields must be persisted in radians.");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe add "(tunit=rad)" to the exception message to specify how.

Copy link
Contributor Author

@parejkoj parejkoj Aug 23, 2016

Choose a reason for hiding this comment

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

I guess supporting us reading in non-radian fields (e.g. from other data sets) is work for another time?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe add "(tunit=rad)" to the exception message to specify how.

Done

I guess supporting us reading in non-radian fields (e.g. from other data sets) is work for another time?

Yes. I actually originally tried to do that here, but hard to roll it back - it turns out we're using non-Angle fields with non-radian units in Wcs persistence, and those changes broke that.

@TallJimbo
Copy link
Member

Is this going to cause difficulties for #84's merge?

I don't think so; the only Python test code here is in a new file that uses the new boilerplate.

These apparently should have never been committed; an alternative
approach was implemented on the same commit (413bacb) where they
first appeared.
@TallJimbo TallJimbo merged commit 0187454 into master Aug 24, 2016
@ktlim ktlim deleted the tickets/DM-7221 branch August 25, 2018 06:44
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

3 participants