search

MESMER-group / mesmer

spatially-resolved ESM-specific multi-scenario initial-condition ensemble emulator
https://mesmer-emulator.readthedocs.io/en/latest/
GNU General Public License v3.0
22 stars 15 forks source link

Harmonic model testing #459

Closed veni-vidi-vici-dormivi closed 1 month ago

veni-vidi-vici-dormivi commented 1 month ago

Increasing the number of years in the testing of the harmonic model fit in the following test

https://github.com/MESMER-group/mesmer/blob/dc60db6b1e154bde73f5c242a71792b458f41224/tests/unit/test_harmonic_model.py#L206-L241

fails. The selected orders increase even though the fit for the coefficients is close to zero. We should check the BIC and why it keeps fitting in this case. The fitting still works on real data, so I'm really not sure what is happening here. Even if it does not change the predictions significatntly, fitting up to the max order on every gridpoint massively slows down the fit.

Things we should check:

mathause commented 1 month ago

No feeling where that is coming from. I would probably start by looking at the BIC and the different constituents.

I just played around with it a bit and the resulting fit is really bad - maybe need to work on this. Need to come up with an easy example to play around with.

mathause commented 1 month ago

Can you check minimize_result.success and then rewrite func to return the residuals instead of the mse? I think then mse = np.mean(minimize_result.fun ** 2).

mathause commented 1 month ago

This tremendously speeds up the tests. But leads to three failures. However, I think it might be the tests.

  1. In test_fit_to_bic_numerical_stability we still use ones and the solution is not stable (i.e. different coeffs lead to the correct preditions) - it works as soon as there is a minor random variation https://github.com/MESMER-group/mesmer/blob/dc60db6b1e154bde73f5c242a71792b458f41224/tests/unit/test_harmonic_model.py#L105
  2. Also the [-1, 1] may be confusing to the algorithm https://github.com/MESMER-group/mesmer/blob/dc60db6b1e154bde73f5c242a71792b458f41224/tests/unit/test_harmonic_model.py#L57

-> do we need to have some restrictions on the values of yearly_predictor?

I also realized that this could be replaced by monthly_target

https://github.com/MESMER-group/mesmer/blob/dc60db6b1e154bde73f5c242a71792b458f41224/tests/unit/test_harmonic_model.py#L130

and

https://github.com/MESMER-group/mesmer/blob/dc60db6b1e154bde73f5c242a71792b458f41224/tests/unit/test_harmonic_model.py#L220

can be collapsed by removing the 'magic comma'

veni-vidi-vici-dormivi commented 1 month ago

can be collapsed by removing the 'magic comma'

Did you count that out? 😄

mathause commented 1 month ago

can be collapsed by removing the 'magic comma'

Did you count that out? 😄

I realized playing around with the above problem. Sorry my pedantry got the better of me...

veni-vidi-vici-dormivi commented 1 month ago

can be collapsed by removing the 'magic comma'

Did you count that out? 😄

I realized playing around with the above problem. Sorry my pedantry got the better of me...

Don't worry, I like your eye for details 😌