GreenBankObservatory / dysh

https://dysh.readthedocs.io
Other
9 stars 3 forks source link

poorly constrained fit in example PS observation #174

Open teuben opened 8 months ago

teuben commented 8 months ago

Describe the bug

Executing the example positionswitch I get this warning when a baseline fit is done

  WARNING: The fit may be poorly conditioned     [astropy.modeling.fitting]

Perhaps the freq axis was not "normalized", it claims it runs in the 1,400,000,000 band, so if the units used are Hz, I could imagine this error. Probably better to do the fit in km/s space, or even better, pick a center freq (or km/s) and fit around that such that the matrix is better behaved?

How to Reproduce

The cell with ta[0].baseline does a baseline fit, and gives this warning.

Environment

mpound commented 8 months ago

what's your astropy version?

teuben commented 8 months ago

5.3.2

teuben commented 8 months ago

when upgraded to 6.0.0 I still get the warning.

mpound commented 8 months ago

I can reproduce this with 0.2.0b.
What do you mean by not normalized? The spectral_axis of the spectrum is in Hz : ta[0].spectral_axis Out[76]: <SpectralAxis (observer to target: radial_velocity=0.0 km / s redshift=0.0 doppler_rest=1420405751.7 Hz doppler_convention=optical) [1.41426369e+09, 1.41426297e+09, 1.41426226e+09, ..., 1.39082833e+09, 1.39082762e+09, 1.39082690e+09] Hz>

Fitting needs to work regardless of what exclude region(s) are chosen and what units the axis are in.

teuben commented 8 months ago

if the fitting is done in straight Hz, the X axis will be around 1.4e9, so the matrix to be inverted will have huge numbers that are all very positive. Better would be to have numbers scaled, like doppler velocities, or simply the mean of the freq's subtracted, and added back later when needed.. Normally one would say rounding errors can become important, but the matrix is probably hard to invert.

teuben commented 3 months ago

It was found rescaling the freq. axis works (e.g. by first freq)

Using model=chebyshev' it also works, since they need a rescaled freq

The differences are quite huge, I put a model 2nd order that went from 0 to 4 at the edge, and the returned spectral still had almost half of that left. A little experimentation showed that normalizing to 1e9, 1e8, 1e7 worked, but already by 1e6 the fitted spectrum had rounding problems. This is for order=2, and is likely worse for higher orders.

@todo This also implies we should at least check what happens if the freq. axis is not regular

@todo model= is not using minimum match, but the docs say there are 2 options, but the error messages when picking something incorrect is ['polynomial', 'chebyshev', 'legendre', 'hermite']

teuben commented 3 months ago

Although this is basically fixed, #260 is throwing a wrench in this, as the normalization in channel space will not correctly understand any other WCS specification.