spacetelescope / synphot_refactor

Synthetic photometry using Astropy
http://synphot.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
39 stars 25 forks source link

TST: filter_par.rst doctest fails in devdeps #374

Closed pllim closed 8 months ago

pllim commented 8 months ago

Was not a problem a few days ago and there has been no code change here since, so must be some incompatibility with unreleased upstream dependency. My bet is on numpy 2.0.dev.

Example log: https://github.com/spacetelescope/synphot_refactor/actions/runs/7867887899/job/21464235574

___________________________ [doctest] filter_par.rst ___________________________
040     >>> n_lambda, lambda_0, delta_lambda, tr_max, fft_pars = filter_to_fft(bp)
041     >>> n_lambda  # Number of elements in wavelengths
042     10000
043     >>> lambda_0  # Starting value of wavelengths  # doctest: +FLOAT_CMP
044     <Quantity 3479.999 Angstrom>
045     >>> delta_lambda  # Median wavelength separation  # doctest: +FLOAT_CMP
046     <Quantity 0.66748047 Angstrom>
047     >>> tr_max  # Peak value of throughput  # doctest: +FLOAT_CMP
048     <Quantity 0.241445>
049     >>> fft_pars  # FFT parameters  # doctest: +FLOAT_CMP
Expected:
    [(407.5180314841658+7.494005416219807e-16j),
     (-78.52240189503877-376.53990235136575j),
     (-294.86589196496584+127.25464850352665j),
     (130.20273803287864+190.84263652863257j),
     (96.62299079012317-91.70087676328245j),
     (-32.572468348727654-34.227696019221035j),
     (-8.051741476066471-21.354793540998294j),
     (-51.708676896903725+6.883836090870033j),
     (13.08719675518801+54.48177212720124j),
     (38.635087381362396-13.02803811279449j)]
Got:
    [(407.5180314841659-4.4896378281755744e-14j),
     (-78.52240189503884-376.5399023513658j),
     (-294.8658919649659+127.25464850352672j),
     (130.20273803287873+190.8426365286326j),
     (96.62299079012317-91.7008767632825j),
     (-32.57246834872768-34.227696019221035j),
     (-8.051741476066466-21.354793540998287j),
     (-51.708676896903725+6.883836090870033j),
     (13.087196755188016+54.48177212720123j),
     (38.6350873813624-13.028038112794505j)]

docs/synphot/filter_par.rst:49: DocTestFailure

https://synphot.readthedocs.io/en/latest/synphot/filter_par.html#generating-fft

https://github.com/spacetelescope/synphot_refactor/blob/master/docs/synphot/filter_par.rst

pllim commented 8 months ago

The most significant change is the first element in the array but I don't know what that means. Maybe @bmorris3 does since the algorithm was ported from tynt.

Old: 407.5180314841658+7.494005416219807e-16j New: 407.5180314841659-4.4896378281755744e-14j

bmorris3 commented 8 months ago

I agree that numpy is the likely culprit, since that's essentially the only relevant dependency here. The change that you noticed is 1% of a float near zero, so I'm pretty surprised that the float comparison filter didn't let this one slide.

I suspect this is the difference: https://numpy.org/devdocs/release/2.0.0-notes.html#numpy-fft-support-for-different-precisions-and-in-place-calculations

pllim commented 8 months ago

Thanks for the info; very useful!

What is the correct behavior here for this algorithm then? Is data precision good enough (I think the test data is stored as float32 in FITS) or should the algorithm up-cast everything to float64 right off the bat?

bmorris3 commented 8 months ago

Hmmm. My position is that the user should decide what precision is sufficient, and if the data are given in 32, the result should be in 32.

Would you like a PR with an update to this docstring?

pllim commented 8 months ago

In that case, my plan is to lock the doctest of this particular page to numpy<2 (so it will be skipped in devdeps) and open up a follow-up issue to unpin that in the future after numpy 2.0 is released. I can do that when I have time. Thanks for your inputs!