Port the baroclinic channel test group from compass

E3SM-Project / polaris

Testing and analysis for OMEGA, MPAS-Ocean, MALI and MPAS-Seaice

BSD 3-Clause "New" or "Revised" License

6 stars 13 forks source link

Port the baroclinic channel test group from compass #27

Closed xylar closed 1 year ago

xylar commented 1 year ago

Checklist

[x] User's Guide has been updated
[x] Developer's Guide has been updated
[x] API documentation in the Developer's Guide (api.md) has any new or modified class, method and/or functions listed
[x] Documentation has been built locally and changes look as expected
[x] Testing comment in the PR documents testing used to verify the changes
[x] New tests have been added to a test suite

xylar commented 1 year ago

Testing

So far, I have only tested the 10 km test cases. On my Ubuntu laptop, I verified that all of the following are bit-for-bit with compass:

default
decomp_test
restart_test
threads_test

I also verified that rpe_test ran successfully and produced the following images that look qualitatively similar to compass output: sections_baroclinic_channel_10 0 rpe_t

I have not yet verified that the RPE tests run at 4 and 1 km resolution. I also haven't compared the RPE output quantitatively with compass. I believe some algorithmic changes I made might affect the results and I think we likely are willing to live with that.

xylar commented 1 year ago

@cbegeman, when you have time after you get back from vacation, could you run the RPE tests for both compass main and this branch, and verify that the output looks similar enough for your taste?

I would appreciate you having a careful look at the code and documentation, too, since this will likely serve as our template for a lot of future development.

xylar commented 1 year ago

@cbegeman, just a quick note to say that I have something I'm happy with in #28 and will use this branch to test it but haven't got that far yet. I'll ping you again when #28 and then this are ready to review.

xylar commented 1 year ago

I noticed that the 1km rpe test ended up with a config_btr_dt = 1s instead of compass's 2s. I think the consistent scaling of dt with resolution is best, just wanted to make you aware.

Yes, I believe I did that on purpose for consistency of the algorithm for computing config_btr_dt. I agree that the change is desirable but worth noting.

xylar commented 1 year ago

@cbegeman, I believe I've made all the changes you requested in your review. I'll go ahead and merge after CI completes. Obviously, we will want to continue to test this and make changes as issues emerge, since this is too much code to hope to port over and modify without bugs.