Skip to content

ERFA not scalar-proof #3135

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.

Sign up for GitHub

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

Jump to bottom
Closed
mhvk wants to merge 1 commit into from
Closed

ERFA not scalar-proof #3135

mhvk wants to merge 1 commit into from

Conversation

@mhvk
Copy link
Contributor

@mhvk mhvk commented Nov 24, 2014

While trying to use the full multi-D flexibility of the new erfa routines in Time, it turns out not all parts can handle scalars:

In [21]: from astropy import erfa

In [22]: erfa.utctai(2400000.,0.)
---------------------------------------------------------------------------
IndexError                                Traceback (most recent call last)
<ipython-input-22-27c7567c1ad0> in <module>()
----> 1 erfa.utctai(2400000.,0.)

/data/mhvk/venv/astropy-dev/lib/python3.4/site-packages/astropy-1.0.dev10476-py3.4-linux-x86_64.egg/astropy/erfa/_erfa.cpython-34m.so in astropy.erfa._erfa.utctai (astropy/erfa/erfa.c:184367)()

/data/mhvk/venv/astropy-dev/lib/python3.4/site-packages/astropy-1.0.dev10476-py3.4-linux-x86_64.egg/astropy/erfa/_erfa.cpython-34m.so in astropy.erfa._erfa.check_errwarn (astropy/erfa/erfa.c:5406)()

IndexError: 0-d arrays can't be indexed

Here, the problem is actually that this value of jd causes a warning; it works fine for a julian date that is within the expected range:

In [23]: erfa.utctai(2450000.,0.)
Out[23]: (array(2450000.0), array(0.0003356481481481266))

@mhvk
Copy link
Contributor Author

mhvk commented Nov 24, 2014

The problem is actually easy to resolve, with two small changes in erfa.pyx.templ... PR coming soon.

@mhvk
Copy link
Contributor Author

mhvk commented Nov 24, 2014

@Etec, @jwoillez - this PR makes a change to erfa.pyx.templ. I converted it into erfa.pyx using cython_generator.py. I'm not quite sure whether I should commit both files, though. Please advice.

@mhvk mhvk mentioned this pull request Nov 24, 2014
Closed
@mhvk mhvk force-pushed the erfa-scalar-warning branch 2 times, most recently from e122ffa to 04a0047 Compare November 24, 2014 18:16
@mhvk mhvk force-pushed the erfa-scalar-warning branch from 04a0047 to c7c6a74 Compare November 24, 2014 19:02
@mhvk mhvk mentioned this pull request Nov 24, 2014
Closed
@mhvk
Copy link
Contributor Author

mhvk commented Nov 24, 2014

@eteq, @jwoillez - would you be able to have a quick look at this PR? Without it, any warning on a scalar becomes an exception... It would help to have this in for the Time conversion work I'm doing... (will be beautifully multi-D!)

@mhvk mhvk mentioned this pull request Nov 24, 2014
Merged
eteq
eteq reviewed Nov 24, 2014
View reviewed changes
astropy/erfa/erfa.pyx.templ
Copy link
Member

@eteq eteq Nov 24, 2014

Choose a reason for hiding this comment

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

Doesn't this have an unintended consequence of silencing warnings when a scalar is passed in? That is, if a scalar statcode is 1, this will convert it to an empty array and just pass through silently?

Copy link
Contributor Author

@mhvk mhvk Nov 25, 2014

Choose a reason for hiding this comment

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

No, the next line with np.unique(statcodes) will still have one entry -- also, I added a test to make sure (since, yes, I did do it wrong several times...).

Copy link
Member

@eteq eteq Nov 25, 2014

Choose a reason for hiding this comment

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

Ah, gotcha, because we're in an if statement that already ensures errors are present.

@eteq
Copy link
Member

eteq commented Nov 24, 2014

@mhvk - now on how to do this: currently what you have to do is commit both erfa.pyx.templ and erfa.pyx. There was some discussion in #2992, and more recently in #3134 that we should make it so that erfa.pyx is auto-generated instead of in the source code. So you might want to wait on #3134, because if it does implement that then there's no need to fill up the revision history with lots of changes to erfa.pyx...

(Also see my question above about the actual code)

@eteq
Copy link
Member

eteq commented Nov 25, 2014

Ok, cool, the code looks good to me. It could be merged as-is, or you may want to wait on the outcome of #3134 ...

@mhvk mhvk mentioned this pull request Nov 27, 2014
Merged
jwoillez added a commit to jwoillez/astropy that referenced this pull request Nov 27, 2014
@jwoillez
Manual cherry pick of PR astropy#3135
ae6b02d 
Credit goes to @mhvk
jwoillez added a commit to jwoillez/astropy that referenced this pull request Nov 27, 2014
@jwoillez
Manual cherry pick of PR astropy#3135
0fdca98 
Credit goes to @mhvk
jwoillez added a commit to jwoillez/astropy that referenced this pull request Nov 27, 2014
@jwoillez
Manual cherry pick of PR astropy#3135
f4a29c0 
Credit goes to @mhvk
@mhvk
Copy link
Contributor Author

mhvk commented Nov 28, 2014

Included in #3141, so closing.

@mhvk mhvk closed this Nov 28, 2014
@mhvk mhvk deleted the erfa-scalar-warning branch November 28, 2014 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mhvk @eteq