Handle missing or stale IERS-A better - #13052

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

I think this is a sensible idea and would hopefully cause minimal pain for people who just want things to work and don't care about extreme precision. We can also have the exceptions point people to a docs page or note that explains how to disable the auto-download, and explains the risk and implications of doing this. I also think that this is sufficiently urgent that we should try to get something in to address this before the next release. I don't have a strong opinion on whether this should be a new configuration keyword or whether we should overload auto_download.

Broad brush I think this is a good idea, but I'm a bit worried about the questionable accuracy of " if the user explicitly opts out of getting the latest IERS data, then they understand what they are doing" 😉... That is, I think this is the sort of thing someone wouldn't necessarily recognize as having the stated effect because it seems like it's about downloading instead of time/coordinates accuracy. So something like either a different name than overloading auto_download or having the "False" in this PR instead be a special sentinal string like conf.auto_download = 'allow_degraded_accuracy'.

Will try to give a more thorough review ASAP - I agree with @adrn this is a high priority to try to get into next release

(and to clarify, I think this in particularly addresses the concern @bjweiner brought up in #12998 (comment), to tie things together a bit more )

@adrn @eteq @mhvk - I updated this PR to add a new config parameter iers_degraded_accuracy. If this looks basically acceptable code-wise then I can work on finishing this up with supporting docs. If you have a better idea for the config parameter name then let me know.

I'm not sure the tests are written in the best way, I honestly have forgotten the details of making sure that there is no IERS data cached somewhere.

One failing test right now is a doctest that is complaining because it is trying to access network resources without +REMOTE_DATA:

044     >>> t = Time('J2010')
045     >>> obj = EarthLocation(-1*u.deg, 52*u.deg, height=10.*u.km)
046     >>> home = EarthLocation(-1*u.deg, 52*u.deg, height=0.*u.km)
047     >>> altaz_frame = AltAz(obstime=t, location=home)
048     >>> obj.get_itrs(t).transform_to(altaz_frame).alt # doctest: +FLOAT_CMP
UNEXPECTED EXCEPTION: IERSWarning('failed to download https://maia.usno.navy.mil/ser7/finals2000A.all: <urlopen error An attempt was made to connect to the internet by a test that was not marked `remote_data`. The requested host was: maia.usno.navy.mil>')
Traceback (most recent call last):

What I don't understand is why this was ever working. If I turn off the network on my laptop and run the doctest example, I get warnings saying that the code is trying to access the IERS site to get finals2000A.all. (And actually I don't quite understand why that is needed for a time of 2010.0, but never mind that).

The only thing I can think is that in my unit test I clear the download cache and maybe it was there before? But that doesn't make sense, I thought the tests are running in a clean (no cache files) environment.

Would it be OK to just add the +REMOTE_DATA doctest flag?

Astropy 5.0

In [1]:     >>> from astropy.coordinates import EarthLocation, AltAz
   ...:     >>> from astropy.time import Time
   ...:     >>> from astropy import units as u
   ...: 
   ...:     >>> t = Time('J2010')
   ...:     >>> obj = EarthLocation(-1*u.deg, 52*u.deg, height=10.*u.km)
   ...:     >>> home = EarthLocation(-1*u.deg, 52*u.deg, height=0.*u.km)
   ...:     >>> altaz_frame = AltAz(obstime=t, location=home)
   ...:     >>> obj.get_itrs(t).transform_to(altaz_frame).alt
WARNING: failed to download ftp://anonymous:mail%[email protected]/pub/products/iers/finals2000A.all and https://datacenter.iers.org/data/9/finals2000A.all, using local IERS-B: <urlopen error Unable to open any source! Exceptions were {'ftp://anonymous:mail%[email protected]/pub/products/iers/finals2000A.all': URLError(gaierror(8, 'nodename nor servname provided, or not known. requested URL: ftp://anonymous:mail%[email protected]/pub/products/iers/finals2000A.all')), 'https://datacenter.iers.org/data/9/finals2000A.all': URLError(gaierror(8, 'nodename nor servname provided, or not known. requested URL: ftp://anonymous:mail%[email protected]/pub/products/iers/finals2000A.all'))}> [astropy.utils.iers.iers]
Out[1]: <Latitude 86.32878441 deg>

In [2]: import astropy

In [3]: astropy.__version__
Out[3]: '5.0'

Thoughts @pllim?

Adding a new configuration item changed the example output at

Awesome @eerovaher thanks! I'll revert the changes I just made.

It looks like the diff code coverage is not running with remote_data. Is that right? The missing coverage it notes is definitely covered by the new tests that require remote data.

@adrn @eteq @mhvk - I think this is fully ready now and should get into the release. Comments welcome.

Thanks @taldcroft - I'll take a look over lunch today. (BTW: @eerovaher is a coordinates maintainer as well, so I also trust his eyes here!)

Hello @taldcroft 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2022-04-21 21:58:52 UTC

@pllim - yup, those narrative docs page need some attention too. Sigh, it's always the docs and testing that take so long!

Thanks for all the feedback all! 🤞 on tests and mostly RTD, but I think that I got all the suggestions in one way or another. @mhvk the 5.1.rst has a straggler long line again from @adrn, someone can fix it in another PR! 😄.

CI is passing now!