Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

poorly constrained fit in example PS observation #174

Open
teuben opened this issue Jan 2, 2024 · 7 comments · May be fixed by #448
Open

poorly constrained fit in example PS observation #174

teuben opened this issue Jan 2, 2024 · 7 comments · May be fixed by #448
Assignees
Labels
bug Something isn't working

Comments

@teuben
Copy link
Collaborator

teuben commented Jan 2, 2024

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

  • Dysh version 0.2.0
  • Python version 3.11.4
  • OS Kubuntu 22.04
@teuben teuben added the bug Something isn't working label Jan 2, 2024
@mpound
Copy link
Collaborator

mpound commented Jan 3, 2024

what's your astropy version?

@teuben
Copy link
Collaborator Author

teuben commented Jan 3, 2024

5.3.2

@teuben
Copy link
Collaborator Author

teuben commented Jan 3, 2024

when upgraded to 6.0.0 I still get the warning.

@mpound
Copy link
Collaborator

mpound commented Jan 4, 2024

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
Copy link
Collaborator Author

teuben commented Jan 5, 2024

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.

@astrofle astrofle assigned astrofle and teuben and unassigned astrofle Apr 24, 2024
@teuben
Copy link
Collaborator Author

teuben commented May 22, 2024

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
Copy link
Collaborator Author

teuben commented May 23, 2024

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.

@astrofle astrofle linked a pull request Jan 16, 2025 that will close this issue
@astrofle astrofle linked a pull request Jan 16, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants