search

NCAR / ccpp-framework

Common Community Physics Package (CCPP)
http://www.dtcenter.org/community-code/common-community-physics-package-ccpp/
Other
26 stars 63 forks source link

initialized_set_block bug fix #503

Closed grantfirl closed 9 months ago

grantfirl commented 10 months ago

When trying to update the latest UFS to use the latest CCPP framework hash, one of the regression tests using CMEPS uses a suite that doesn't have any schemes with a non-empty init phase. Due to this, the subroutine generated for the init phase in the cap doesn't have access to a ccpp_t instance. Yet, in mkstatic.py a line like initialized({ccpp_var_name}%ccpp_instance) = .true. is put in the subroutine regardless, resulting in a compilation error.

A fix is to pass the ccpp_t_instance into the group cap for the init phase when it is otherwise empty so that initialized({ccpp_var_name}%ccpp_instance) = .true. has access to the ccpp_t_instance.

User interface changes?: No

Fixes: No issue started

Testing: test removed: unit tests: system tests: UFS RTs (re-running as of 2023/10/12), SCM RTs (completed successfully) manual testing:

grantfirl commented 9 months ago

@mkavulich @climbfuji @gold2718 This is a bugfix for https://github.com/NCAR/ccpp-framework/pull/463 and this must be merged before the UFS can use the latest ccpp-framework commit.

climbfuji commented 9 months ago

I think that's ok, but I am not entirely familiar with how the NRL are using the member functionality (for which initialized was turned into an array). Is it potentially dangerous to set all members to initialize because we don't have enough information on the member for which this is called?

It would have been more along the lines of ccpp_prebuild if the initialized_set_block was a formatted string like the others (Group.initialized_set_blocks['fallback'] maybe, same for test_blocks)?

@michalakes can you comment please?

michalakes commented 9 months ago

I think that's ok, but I am not entirely familiar with how the NRL are using the member functionality (for which initialized was turned into an array). Is it potentially dangerous to set all members to initialize because we don't have enough information on the member for which this is called? It would have been more along the lines of ccpp_prebuild if the initialized_set_block was a formatted string like the others (Group.initialized_set_blocks['fallback'] maybe, same for test_blocks)?

@michalakes can you comment please?

I'm out of the office until next week but I'll test to make sure this proposed fix does no harm for the NRL codes and let you know.

grantfirl commented 9 months ago

I think that's ok, but I am not entirely familiar with how the NRL are using the member functionality (for which initialized was turned into an array). Is it potentially dangerous to set all members to initialize because we don't have enough information on the member for which this is called?

It would have been more along the lines of ccpp_prebuild if the initialized_set_block was a formatted string like the others (Group.initialized_set_blocks['fallback'] maybe, same for test_blocks)?

You make a really good point about the danger of setting the entire initialized array. While it fixes the UFS problem, it may introduce another for NRL. I've updated the fix to just pass in the ccpp_t instance (cdata) to the init subroutine in the group cap (when it is otherwise empty) so that it can properly be referenced by the initialized set statement as originally intended. I think that this should be a better solution.

I'm re-testing in UFS to make sure, but @climbfuji @michalakes @gold2718 please take a look at the updated fix when you get a chance.

grantfirl commented 9 months ago

Passed UFS regression test that was failing initially. I'm rerunning the rest now...

grantfirl commented 9 months ago

Passed all UFS RTs on Hera except control_p8_gnu which is likely a different, unrelated issue: https://github.com/ufs-community/ufs-weather-model/issues/1939

mkavulich commented 9 months ago

@michalakes Would you be able to test the latest fixes here? Just ensuring this won't cause any problems on the NRL side.

michalakes commented 9 months ago

@michalakes Would you be able to test the latest fixes here? Just ensuring this won't cause any problems on the NRL side.

Yes, sorry. I got sidetracked this week. Please stand by...

michalakes commented 9 months ago

I tested fix with NEPTUNE and "does no harm". I wasn't able to update to the latest CCPP because of some other issues in progress, but I spliced in the set of changes to external/ccpp/ccpp-framework/scripts/mkstatic.py and tested with one of the cases we have that exercise two domains. That worked as before. So... thumbs up from our end. Thanks very much for checking with us on this! And sorry for the delay. -John

mkavulich commented 9 months ago

Thanks for confirming @michalakes! Merging now.