Skip to content

ERFA not scalar-proof - #3135

Closed
mhvk wants to merge 1 commit into
astropy:masterfrom
mhvk:erfa-scalar-warning
Closed

ERFA not scalar-proof#3135
mhvk wants to merge 1 commit into
astropy:masterfrom
mhvk:erfa-scalar-warning

Conversation

@mhvk

@mhvk mhvk commented Nov 24, 2014

Copy link
Copy Markdown
Contributor

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

mhvk commented Nov 24, 2014

Copy link
Copy Markdown
Contributor Author

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

@mhvk

mhvk commented Nov 24, 2014

Copy link
Copy Markdown
Contributor Author

@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

mhvk commented Nov 24, 2014

Copy link
Copy Markdown
Contributor Author

@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
Comment thread astropy/erfa/erfa.pyx.templ

@eteq eteq Nov 24, 2014

Copy link
Copy Markdown
Member

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?

@mhvk mhvk Nov 25, 2014

Copy link
Copy Markdown
Contributor Author

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...).

@eteq eteq Nov 25, 2014

Copy link
Copy Markdown
Member

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

eteq commented Nov 24, 2014

Copy link
Copy Markdown
Member

@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

eteq commented Nov 25, 2014

Copy link
Copy Markdown
Member

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

mhvk commented Nov 28, 2014

Copy link
Copy Markdown
Contributor Author

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