search

ESCOMP / MOSART

Model for Scale Adaptive River Transport, Mosart, part of the Community Earth System Model
http://www.cesm.ucar.edu/
Other
8 stars 27 forks source link

bring in nuopc cap #25

Closed mvertens closed 5 years ago

mvertens commented 5 years ago

This PR brings in the NUOPC cap addition into master. New mosart baselines were created on izumi and the following tests were run:

./create_test --xml-category mosart --xml-machine hobart --compare mosart1_0_33_ctsm1.0.dev049

The tests passed and the results were bfb. Note that only the mct tests were run here. Future tests will include the nuopc cap.

mvertens commented 5 years ago

@ekluzek - could you please let me know when this PR will be merged in. I believe I have done all of the relevant testing and it does not effect the mct answers. This is really needed for the high-priority NUOPC work that is ongoing.

ekluzek commented 5 years ago

OK, sorry. I have been looking at this. I might have some changes to request, and/or may need to ask you some questions. So I'll get back to you again by this afternoon.

mvertens commented 5 years ago

Thanks. This code does not need a code review in my opinion. I have also gone over these changes previously with @billsacks and he was fine with the changes. What is needed is verification that enough testing was done and that the PR can then be merged.

On Fri, Jul 26, 2019 at 10:31 AM Erik Kluzek notifications@github.com wrote:

OK, sorry. I have been looking at this. I might have some changes to request, and/or may need to ask you some questions. So I'll get back to you again by this afternoon.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/mosart/pull/25?email_source=notifications&email_token=AB4XCE5IUTULYUAGP7WKXELQBMRHVA5CNFSM4IEJLYZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD25DBWI#issuecomment-515518681, or mute the thread https://github.com/notifications/unsubscribe-auth/AB4XCE3JWPBOFDXX2KITMCDQBMRHVANCNFSM4IEJLYZA .

ekluzek commented 5 years ago

It's also good to do code reviews with more people. But, I do think you can see that your doing a code review with Bill doesn't help me to understand the changes. I thought the changes might be just in adding new code to nuopc, but there's also changes on the MCT side. It's helpful for me to understand what's going on there.

But, I've gone through and figured it out and updated the ChangeLog now.

mvertens commented 5 years ago

The changes were needed since the model code was referencing cap code - which should never happen. The same changes will be brought into the rtm PR that will be coming soon.

On Fri, Jul 26, 2019 at 11:37 AM Erik Kluzek notifications@github.com wrote:

It's also good to do code reviews with more people. But, I do think you can see that your doing a code review with Bill doesn't help me to understand the changes. I thought the changes might be just in adding new code to nuopc, but there's also changes on the MCT side. It's helpful for me to understand what's going on there.

But, I've gone through and figured it out and updated the ChangeLog now.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/mosart/pull/25?email_source=notifications&email_token=AB4XCE4JW4ECCAG26OLGPC3QBMY4PA5CNFSM4IEJLYZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD25H5QQ#issuecomment-515538626, or mute the thread https://github.com/notifications/unsubscribe-auth/AB4XCE7B34YTW6UY4V2FQGLQBMY4PANCNFSM4IEJLYZA .

mvertens commented 5 years ago

I think this is a worthwhile goal. Moving forwards - once we get all of the nuopc caps into place we can see if it makes sense to do this. However, as you noted there is very little overlap.

On Fri, Jul 26, 2019 at 11:48 AM Erik Kluzek notifications@github.com wrote:

@ekluzek approved this pull request.

OK, I've gone through and figured this out.

I think there are some points where a few more comments would be helpful for some of the new code. But, I think it's OK.

As a future change I also wonder if there can be a shared "mosart_comp.F90" that could have some shared init, run, final subroutines for mosart specific parts that aren't dependent on which driver is used. It looks like right now this would be small, so maybe doesn't matter. But, that is a good principle to use if possible -- lower the duplication and use shared code whenever possible.

The other thing that will need to be added in at some point are some tests to make sure the nuopc driver is working. So a test-mods directory for turning nuopc driver on should be added.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/mosart/pull/25?email_source=notifications&email_token=AB4XCEY6GMNHSEWHTQQAFB3QBM2GRA5CNFSM4IEJLYZKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB7XP7NA#pullrequestreview-267321268, or mute the thread https://github.com/notifications/unsubscribe-auth/AB4XCEYDLEFBLYKDPPBFEMTQBM2GRANCNFSM4IEJLYZA .

mvertens commented 5 years ago

This is very easily doable. We just add --driver nuopc to create_test. I agree that this is a good thing to do.

On Fri, Jul 26, 2019 at 11:50 AM Erik Kluzek notifications@github.com wrote:

Merged #25 https://github.com/ESCOMP/mosart/pull/25 into master.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/mosart/pull/25?email_source=notifications&email_token=AB4XCE6YEMIDB4IF3FMW66LQBM2PFA5CNFSM4IEJLYZKYY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOSXG37XA#event-2513289180, or mute the thread https://github.com/notifications/unsubscribe-auth/AB4XCE76LGZFBDKV2YT42C3QBM2PFANCNFSM4IEJLYZA .

mvertens commented 5 years ago

I am doing this in all of my testing.

On Fri, Jul 26, 2019 at 11:52 AM Mariana Vertenstein mvertens@ucar.edu wrote:

This is very easily doable. We just add --driver nuopc to create_test. I agree that this is a good thing to do.

On Fri, Jul 26, 2019 at 11:50 AM Erik Kluzek notifications@github.com wrote:

Merged #25 https://github.com/ESCOMP/mosart/pull/25 into master.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/mosart/pull/25?email_source=notifications&email_token=AB4XCE6YEMIDB4IF3FMW66LQBM2PFA5CNFSM4IEJLYZKYY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOSXG37XA#event-2513289180, or mute the thread https://github.com/notifications/unsubscribe-auth/AB4XCE76LGZFBDKV2YT42C3QBM2PFANCNFSM4IEJLYZA .

ekluzek commented 5 years ago

In order to ensure that both the mct cap and nuopc caps are tested, there should be a testmods_dirs/mosart/nuopcCAP directory added that has a include_user_mods that starts from "../default" and then adds a shell_commands file with

./xmlchange COMP_INTERFACE=nuopc

Then we know the nuopc cap is tested every time standard testing is run.

mvertens commented 5 years ago

No - I do not think this is needed. Otherwise you are simply duplicating tests. You simply need to invoked your create_test with an optional input of --driver nuopc.

On Fri, Jul 26, 2019 at 2:35 PM Erik Kluzek notifications@github.com wrote:

In order to ensure that both the mct cap and nuopc caps are tested, there should be a testmods_dirs/mosart/nuopcCAP directory added that has a include_user_mods that starts from "../default" and then adds a shell_commands file with

./xmlchange COMP_INTERFACE=nuopc

Then we know the nuopc cap is tested every time standard testing is run.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/mosart/pull/25?email_source=notifications&email_token=AB4XCE7VCXY35GAVQ5GRMR3QBNNY7A5CNFSM4IEJLYZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD25UYDY#issuecomment-515591183, or mute the thread https://github.com/notifications/unsubscribe-auth/AB4XCE2CBUPJILE7AS43GWTQBNNY7ANCNFSM4IEJLYZA .

ekluzek commented 5 years ago

OK, I think we need to talk about this in person then -- because, that makes no sense to me. I'm not going to know I need to run create_test both with and without --driver nuopc. And running the full test list with both drivers means you are duplicating ALL tests for both drivers. So how is that duplicating tests? I'm going to assume I just need one extra test with the nuopc driver.

mvertens commented 5 years ago

You really do want to duplicate all the tests with the NUOPC caps - since things are very different and this is only temporary as we migrate the whole system to the nuopc cap. So its just a function of invoking the create_test twice. Happy to talk more about this next week. And maybe with the other CTSM SE's so that its not just pairwise communication.

On Fri, Jul 26, 2019 at 2:42 PM Erik Kluzek notifications@github.com wrote:

OK, I think we need to talk about this in person then -- because, that makes no sense to me. I'm not going to know I need to run create_test both with and without --driver nuopc. And running the full test list with both drivers means you are duplicating ALL tests for both drivers. So how is that duplicating tests? I'm going to assume I just need one extra test with the nuopc driver.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/mosart/pull/25?email_source=notifications&email_token=AB4XCEZVGMVIN5SFOKULGCLQBNOTBA5CNFSM4IEJLYZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD25VHRQ#issuecomment-515593158, or mute the thread https://github.com/notifications/unsubscribe-auth/AB4XCE2JZ66TZXUPG377FY3QBNOTBANCNFSM4IEJLYZA .

billsacks commented 5 years ago

I agree pretty strongly with @ekluzek here.

billsacks commented 5 years ago

To elaborate... and thinking mostly about CTSM here: We'll want to make sure we regularly test the nuopc cap, but we really don't want to run the full test suite twice for every tag. We need some guidance on a minimal set of tests that is sufficient to cover all of the logic that is unique to the nuopc cap: there's no need to cover all of the myriad of options with both mct and nuopc, since most of the CTSM (and MOSART, RTM, etc.) code is the same whether you're using mct or nuopc.

mvertens commented 5 years ago

I agree. However, at the very beginning I think you probably want to do that. I've caught very strange problems in running the whole test POP aux suite with the mct and nuopc cap. It might be that you simply fire it off as a sanity check and document things. Like I said - I think it would be helpful to talk next week and all come to an agreement of the best way forwards. However, I don't have the CISM cap fully working yet and have not even come close to generating all the mesh files for CTSM - so this is not a time critical issue to resolve.

On Fri, Jul 26, 2019 at 3:27 PM Bill Sacks notifications@github.com wrote:

To elaborate... and thinking mostly about CTSM here: We'll want to make sure we regularly test the nuopc cap, but we really don't want to run the full test suite twice for every tag. We need some guidance on a minimal set of tests that is sufficient to cover all of the logic that is unique to the nuopc cap: there's no need to cover all of the myriad of options with both mct and nuopc, since most of the CTSM (and MOSART, RTM, etc.) code is the same whether you're using mct or nuopc.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/mosart/pull/25?email_source=notifications&email_token=AB4XCEYMCZJHDXWGVME4Y5LQBNT4FA5CNFSM4IEJLYZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD25YB4Q#issuecomment-515604722, or mute the thread https://github.com/notifications/unsubscribe-auth/AB4XCE27P337KQLJVMZSNX3QBNT4FANCNFSM4IEJLYZA .

billsacks commented 5 years ago

@mvertens good point that it's probably worth running the full test suites at least once with the nuopc cap.