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

Add MESMER-M integration tests #501

Open
wants to merge 78 commits into
base: main
Choose a base branch
from

Conversation

veni-vidi-vici-dormivi
Copy link
Collaborator

@veni-vidi-vici-dormivi veni-vidi-vici-dormivi commented Aug 20, 2024

  • Tests added
  • Fully documented, including CHANGELOG.rst

@mathause
Copy link
Member

Thanks - can you save the coeffs as netCDF and not pickle (I know it's a bit more annoying but we want to move away from pickle)

@mathause
Copy link
Member

mathause commented Aug 23, 2024

I haven't looked into it so not sure what is going on but a bit strange that none of the environments get it right. I thought this does not have to be high priority but then it would be good to stabilize the numerics for memser-m. What helped for the covariance thingy was testing the code on cfc vs exo, which have different cpus with different sets of SIMD instructions. But I am not really working today - if I find time I'll take a look tomorrow or else after our retreat...

@veni-vidi-vici-dormivi
Copy link
Collaborator Author

Okay I think (for the moment) this is good enough. I can open an issue on this after merging so we can come back to it maybe in the future. Also, should I separate the changes in the harmonic model and the power transformer from the integration tests into (3) separate PRs or just rename this PR to "MESMER-M integration test stabilizing"?

Copy link
Member

@mathause mathause left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - that's very good to have. I add some first suggestions - the biggest being to not merge the coeffs - let me know if you don't think that's a good idea.

mesmer/stats/_harmonic_model.py Outdated Show resolved Hide resolved
mesmer/stats/_harmonic_model.py Outdated Show resolved Hide resolved
AR1_fit.residuals, weights, phi_gc_localizer, "time", 30
)

# merge into one dataset
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a huge fan of this there and back renaming and merging. Would it be bad to save each Dataset as individual netCDF? Or merge into a datatree? (But maybe later).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of having one dataset with all the calibrated parameters per ESM. I did some renaming in the modules such that everything can be merged without problems. What do you think about that?

tests/unit/test_harmonic_model.py Show resolved Hide resolved
tests/integration/test_draw_mesmer_m.py Show resolved Hide resolved
tests/integration/test_draw_mesmer_m.py Outdated Show resolved Hide resolved
tests/integration/test_draw_mesmer_m.py Outdated Show resolved Hide resolved
tests/integration/test_calibrate_mesmer_m.py Outdated Show resolved Hide resolved
@mathause
Copy link
Member

Oh and could you merge main - unfortunately you will run into some merge conflicts from #508 and #512

@mathause
Copy link
Member

mathause commented Oct 1, 2024

Do you happen to know if the estimates are stable for each OS? Could we do os-dependent tolerances? (Not asking you to sink more time into this PR atm...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants