Skip to content

fix division by zero warnings for values near sun - #10881

Merged
eteq merged 2 commits into
astropy:masterfrom
StuartLittlefair:ld_bugfix
Oct 16, 2020
Merged

eteq merged 2 commits into
astropy:masterfrom
StuartLittlefair:ld_bugfix

Conversation

@StuartLittlefair

Copy link
Copy Markdown
Contributor

The changes to light deflection in #10666 caused some problems for the actual centre of the Sun. In that case the code raises division by zero warnings which can break continuous integration tools. These warnings are also ugly and unnecessary since any affected vectors are replaced with the original directions.

As suggested by @ayshih we wrap these lines in np.errstate to prevent the warnings.

#10666 is currently milestoned for 4.1, so this has been given the same milestone.

Fixes #10878

@StuartLittlefair StuartLittlefair added this to the v4.1 milestone Oct 16, 2020
@StuartLittlefair
StuartLittlefair requested review from eteq and mhvk October 16, 2020 10:38
@pllim pllim added Bug Affects-dev PRs and issues that do not impact an existing Astropy release and removed no-changelog-entry-needed labels Oct 16, 2020

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks good. Tests are failing because of an unrelated issue (see #10874).

@eteq - this will need to join #10666 for backporting to 4.0.3

@mkbrewer

Copy link
Copy Markdown
Contributor

Use of erfa.pn() would take care of this also.

@mhvk

mhvk commented Oct 16, 2020

Copy link
Copy Markdown
Contributor

indeed, erfa.pn is a logical choice here - @StuartLittlefair - obviously not a big deal, but maybe an idea to change it? It will do the right thing, with xyz as the last dimension.

@StuartLittlefair

Copy link
Copy Markdown
Contributor Author

erfa.pn is a nice solution, but it doesn't handle shapes perfectly and I lack @mhvk's numpy wizardry to be sure of doing the right thing. e.g suppose the q array is (N, 3), the way I do it gives sundist a shape of (N, 1) so I can use it in np.where later.

If I use erfa.pn then sundist has shape (N,) - so I could use sundist[:, np.newaxis] but then that doesn't work for scalars? Does the solution below work?

sundist, q = erfa.pn(q)
sundist = np.atleast_1d(sundist)[:, np.newaxis]
q = np.where(sundist > 1.0e-10, q, pco)

@mhvk

mhvk commented Oct 16, 2020

Copy link
Copy Markdown
Contributor

@StuartLittlefair - it would be sundist[..., np.newaxis] to append at the end (works for scalars too):

erfa.pn([1.,2.,3.])[0][..., np.newaxis]
Out[15]: array([3.74165739])
erfa.pn(np.ones((2,2,3)))[0][..., np.newaxis]
Out[16]: 
array([[[1.73205081],
        [1.73205081]],

       [[1.73205081],
        [1.73205081]]])

@StuartLittlefair

Copy link
Copy Markdown
Contributor Author

God bless you @mhvk and your numpy-Fu. And thanks @mkbrewer for the erfa reminder...

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nicer, indeed!

@eteq eteq left a comment

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.

CI failure is unrelated, so I think this is all good and can be backported to 4.1 once the issues still in the #10666 backport are ironed out.

@eteq
eteq merged commit 65ebf4d into astropy:master Oct 16, 2020
mhvk pushed a commit to mhvk/astropy that referenced this pull request Oct 16, 2020
@mhvk mhvk mentioned this pull request Oct 16, 2020
eteq added a commit to eteq/astropy that referenced this pull request Oct 19, 2020
fix division by zero warnings for values near sun
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Affects-dev PRs and issues that do not impact an existing Astropy release Bug coordinates zzz 💤 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.

Improved light-deflection code can have issues for the center of the Sun

5 participants