desihub / specsim

Quick simulations of spectrograph response
2 stars 9 forks source link

Minor PR to fix issue #119. #120

Closed moustakas closed 2 years ago

moustakas commented 2 years ago

Fixes #119.

sbailey commented 2 years ago

Do you understand why

exposure_midpoint = self.exposure_start + 0.5 * self.exposure_time

generates an overflow in the first place? This PR silences the warning, but it doesn't smell like an operation that should overflow.

moustakas commented 2 years ago

Indeed, I can't reproduce this exception from scratch (using astropy v5.0). Using the values in the unit tests I get:

import astropy.time
import astropy.units as u

exposure_start = astropy.time.Time(55000.5, format='mjd')
exposure_time = 1000. * u.s
exposure_start + 0.5 * exposure_time
  <Time object: scale='utc' format='mjd' value=55000.50578703704>

exposure_start, exposure_time
  (<Time object: scale='utc' format='mjd' value=55000.5>, <Quantity 1000. s>)

The exception also only happens once (the first time) when testing this bit of the unit tests.

dkirkby commented 2 years ago

This operation inevitably leads to some loss of precision since exposure_time / exposure_start ~ 1e-7, but that was always true in previous versions, and float64 still retains useful precision for the result.

I don't see any potential overflow issue here with max double ~ 8.9885e+307.

moustakas commented 2 years ago

Digging far too deep into astropy, in addition to waking up a Balrog I also found the issue to be here: https://github.com/astropy/astropy/blob/main/astropy/utils/iers/iers.py#L97-L102

from astropy.utils.iers.iers import _none_to_float
_none_to_float(None)
  1.7976931348623157e+308

It's this value which gets propagated into the various time utilities and causes the overflow.

moustakas commented 2 years ago

This None is the astropy.utils.iers.iers.Conf().auto_max_age attribute, which gets set here-- https://github.com/astropy/astropy/blob/main/astropy/utils/iers/iers.py#L121-L125

auto_max_age = _config.ConfigItem(
    30.0,
    'Maximum age (days) of predictive data before auto-downloading. '
    'See "Auto refresh behavior" in astropy.utils.iers documentation for details. '
    'Default is 30.')

Oh but it looks like we set it to None in desiutil.iers.freeze_iers-- https://github.com/desihub/desiutil/blob/master/py/desiutil/iers.py#L116

And indeed I just tested that if I set this to 30 (the nominal default in astropy) then the exception goes away in the desisim unit tests. However, I don't know what the implications of this change are. @sbailey @dkirkby @weaverba137

weaverba137 commented 2 years ago

Remind me again how a configuratiom item that relates to http downloads is being used in a calculation?

moustakas commented 2 years ago

Remind me again how a configuratiom item that relates to http downloads is being used in a calculation?

Well, that's for the astropy developers to answer.

Here's a MWE with numpy 1.20.3 and astropy 5.0:

import astropy.time
import astropy.units as u
from desiutil.iers import freeze_iers

freeze_iers()
astropy.time.Time(55000.5, format='mjd') + 0.5 * 1000. * u.s

Output:

INFO:iers.py:82:freeze_iers: Freezing IERS table used by astropy time, coordinates.
/Users/ioannis/opt/anaconda3/envs/desi/lib/python3.8/site-packages/astropy/time/utils.py:178: RuntimeWarning: overflow encountered in double_scalars
  c = 134217729. * a  # 2**27+1.
/Users/ioannis/opt/anaconda3/envs/desi/lib/python3.8/site-packages/astropy/time/utils.py:180: RuntimeWarning: invalid value encountered in double_scalars
  ah = c - abig
Out[1]: <Time object: scale='utc' format='mjd' value=55000.50578703704>

Note:

weaverba137 commented 2 years ago

I honestly think this is a red herring, but I'll have to take a closer look on Monday.

weaverba137 commented 2 years ago

In particular None should trigger a TypeError, not an overflow.

moustakas commented 2 years ago

astropy.utils.iers.iers._none_to_float converts the None to 1.7976931348623157e+308, but that value then causes problems in the downstream astropy code, including (eventually) the overflow.

weaverba137 commented 2 years ago

Let me take a look on Monday.

weaverba137 commented 2 years ago

The warnings (not exceptions) that you are seeing come from this line: https://github.com/astropy/astropy/blob/2984fa2ea5f3604b71df8ca687c59d34913e62f1/astropy/utils/iers/iers.py#L986. Yes, it is true that auto_max_age is being used in a calculation, but based on the examples above, I don't see how it is doing anything except generate some warnings.

In particular, in the examples above, I don't see any actual problems with the calculation you are trying to do. For example, Out[1]: <Time object: scale='utc' format='mjd' value=55000.50578703704> appears to be a valid time object.

By the way, recall that most Python warnings are suppressed after the first time they are issued so that's why you only see the warning once.

moustakas commented 2 years ago

Thanks, @weaverba137, I agree with everything you've written, which is consistent with my own sleuthing yesterday. My original fix was to simply suppress the warning but @sbailey asked me to dig and understand the origin of the warning, which I think we've now done. I propose we simply merge this PR.

sbailey commented 2 years ago

Thanks for confirming that this is a false-positive warning bug and that we aren't actually overflowing on that calculation. Merging now.