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

add a way to sidestep common invalid but interpretable units in CCDData #9397

Merged
merged 2 commits into from Oct 23, 2019

Conversation

eteq
Copy link
Member

@eteq eteq commented Oct 17, 2019

This PR addresses an annoyance I've encountered several times now. It's nominally a dupe #7608 but it's also related to #7619.

Arguably, this PR is actually counter to some of the discussion in #7619 but I'm raising it because it's not clear #7619 got resolved, and I keep running into this particular case. So my concrete proposal in this PR is to have a very explicit set of "known bad but unambiguous" units that CCDData knows how to translate. I'm not proposing we promise that this list is complete or anything like that, but instead to think of it more like the from_name machinery in SkyCoord, which is very convenient even if it isn't always 100% reliable.

So basically this PR sets up machinery to do this, and populates with the specific case of #7608 with the expectation that we can add more in the future as users encounter them. I'd say they have to justify them, which I'm doing here via #7608.

cc @mwcraig @MSeifert04

@eteq eteq added the nddata label Oct 17, 2019
@eteq eteq added this to the v4.0 milestone Oct 17, 2019
Copy link
Member

@mwcraig mwcraig left a comment

Choose a reason for hiding this comment

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

Given https://github.com/astropy/astropy/pull/9397/files#diff-927444a7463d962e28a3e4db8f89dcedR613 this approach seems completely fine -- basically, we already do special casing for one unit ADUadu so adding more makes sense to me. If we don't handle the common cases smoothly then users aren't going to use it.

While it is the case, as discussed in #7619, that one could take this too far there simply aren't that many variants (leaving aside spectra) that I can think of.

(I don't think this requires additional documentation, just to be explicit about that point).

I've restarted the job that had failed during apt install....assuming this passes it is ready to go.

@bsipocz
Copy link
Member

bsipocz commented Oct 23, 2019

restarted the cancelled tests, and added the label based on the review above.

@bsipocz bsipocz added the 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Oct 23, 2019
@bsipocz bsipocz merged commit 5ea9a8f into astropy:master Oct 23, 2019
@bsipocz
Copy link
Member

bsipocz commented Oct 23, 2019

Thank you @eteq!

@eteq eteq deleted the allow-electron-s branch October 25, 2019 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nddata 💤 merge-when-ci-passes Do not use: We have auto-merge option now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants