search

nextsimhub / nextsimdg

neXtSIM_DG : next generation sea-ice model with DG
https://nextsim-dg.readthedocs.io/en/latest/?badge=latest
Apache License 2.0
10 stars 13 forks source link

Reinstantiate Dynamic Tests and add to CI #556

Open MarionBWeinzierl opened 1 month ago

MarionBWeinzierl commented 1 month ago

Reinstantiate Dynamic Tests and add to CI

Fixes #549

Task List

Change Description

This PR closes #549. In order to make sure that the upcoming MPI implementation does not break the dynamics code, and existing implementation of dynamics tests is retrieved from an earlier version of this repository. The CMakeLists file needs adapting to make them run again. Then, they are added to the Continuous Integration implementation in Github Actions. Additional tests already present in the dynamics directory are also added to Github Actions.

The added new/renewed tests test the advection part of the dynamics.

Documentation Impact

The README in the dynamics/test folder has been updated.

Pre-Request Checklist

MarionBWeinzierl commented 1 month ago

Info from @winzerle :

I just checked and they are in principle working. And example1 and example 2(don’t know why I called them examples and not tests) should be the ones to start with.

They both test the advection, 2 is on a domain with periodic boundary data.

There is a strange automatic check on whether the result is ok and at the moment the tests all fail. But this is due to their sensitivity. The implementation is correct but there were just slight adjustments over the time that changed the values.

MarionBWeinzierl commented 1 month ago

@winzerle says the "orcamesh" tests are doing the same as the "sasipmesh" tests (just reading in the mesh), therefore I have removed those.

MarionBWeinzierl commented 1 month ago

After the previous changes had introduced artefacts from the new boundary handling, these have now been reverted. The new boundary handling will be added in a separate PR by @winzerle .

MarionBWeinzierl commented 2 weeks ago

@winzerle @timspainNERSC @einola , looking at https://github.com/nextsimhub/nextsimdg/blob/dynamictests/dynamics/test/README.md , is there anything in the README that is still relevant, and could you help with updating it to account for explaining the tests in https://github.com/nextsimhub/nextsimdg/tree/dynamictests/dynamics/test ?

Also, I have renamed "example1" and "example2" to "test1" and "test1", but it would be better to give them names that are more self-explanatory. @winzerle , would "advectiontest1" or something like that fit? And what is the difference between 1 and 2?

winzerle commented 2 weeks ago

Hi,

Both of them are tests of the advection scheme. The second one has periodic boundaries as ’new feature’ So advectiontest is fine and maybe you could add the label periodic to the second one.

I’ll have a look at the readme. I’m sure, it’s mostly old.

Thomas

On 10. Jun 2024, at 12:43, Marion @.***> wrote:

@winzerle @timspainNERSC @einola , looking at https://github.com/nextsimhub/nextsimdg/blob/dynamictests/dynamics/test/README.md , is there anything in the README that is still relevant, and could you help with updating it to account for explaining the tests in https://github.com/nextsimhub/nextsimdg/tree/dynamictests/dynamics/test ? Also, I have renamed "example1" and "example2" to "test1" and "test1", but it would be better to give them names that are more self-explanatory. @winzerle , would "advectiontest1" or something like that fit? And what is the difference between 1 and 2? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

MarionBWeinzierl commented 2 weeks ago

OK, I seem to have to set the tolerance (the variable wasn't used, btw) to a value > 1 for the tests to pass -- @winzerle , this is probably not what we want? Can you confirm that they all pass for 1e-4 for you? Also, which parameters can I tweak to speed the tests up a bit, maybe cutting some of the tests that do not give additional insight or so?

MarionBWeinzierl commented 2 weeks ago

@TomMelt @timspainNERSC , I assume we want those tests also to adhere to the doctest framework? Or are you happy for them to be added as is atm?

MarionBWeinzierl commented 2 weeks ago

(note that the tests say "passed" atm although the advection ones have failed, as the result is currently just communication over written output)

MarionBWeinzierl commented 1 week ago

@TomMelt @timspainNERSC , I assume we want those tests also to adhere to the doctest framework? Or are you happy for them to be added as is atm?

I decided to implement minimal doctest functionality for the two tests for the time being, and added issue #587 for fully refactoring those tests.

This should make CI fail at this point (see comments above).

MarionBWeinzierl commented 1 hour ago

The current status is that all test pass, except from one of the last advection periodic boundary tests on Mac -- it passes fine on Ubuntu. @winzerle is currently trying to look into this, but struggling to get it to build on Mac -- @timspainNERSC could reproduce the error on this Mac, and Thomas has asked for his help now.