NCAR / ccpp-framework

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

Refactor capgen branch #410

Closed gold2718 closed 2 years ago

gold2718 commented 2 years ago

Refactor capgen branch

User interface changes?: Yes

Fixes: [Github issue #s] addresses #329 addresses #378 addresses #403 fixes #408

Testing:

gold2718 commented 2 years ago

From our previous conversation, my understanding was that optional Fortran variables may be omitted from metadata tables, but they can be present. It's a hardcoded choice which flavor of a scheme to run.

Oops, bad communication. capgen does work this way, I added a test in capgen_test to verify that an optional Fortran variable which is included in the metadata table is handled properly.

gold2718 commented 2 years ago

Do you think the improved error handler will be ready before we transition the UFS to capgen?

I have not prioritized this work because it would delay unification readiness. We need to re-evaluate the schedule if you want to add this.

gold2718 commented 2 years ago

I think at least one more Python file needs the Python3 shebang:

Yep, I already found this by testing on a different machine (it turns out that the machine I was using had python ==> python3 so I did not see the problem. Unfortunately, in trying to clean up all this test stuff, I ended up touching a bunch of files so the PR grew.

gold2718 commented 2 years ago

In general: This PR is extremely difficult if not impossible to review in a reasonable amount of time.

I agree with this comment and am happy to try and make shorter PRs. However, if we agree that I will do this, I need to go in and re-estimate all the task hours as a PR does take me a significant amount of time. Your call.

climbfuji commented 2 years ago

In general: This PR is extremely difficult if not impossible to review in a reasonable amount of time.

I agree with this comment and am happy to try and make shorter PRs. However, if we agree that I will do this, I need to go in and re-estimate all the task hours as a PR does take me a significant amount of time. Your call.

It's a judgement call, I think we need to be pragmatic. Maybe along the lines "I can split this up easily without spending a lot of time, so let's to it" and "This would be a lot of overhead, let's keep it together".

climbfuji commented 2 years ago

I ran the tests with your latest commit bbb668c on my Mac, and all tests passed.

climbfuji commented 2 years ago

@gold2718 I am ok with this PR, do you want to make more changes or should I approve and merge? Thanks.

gold2718 commented 2 years ago

@gold2718 I am ok with this PR, do you want to make more changes or should I approve and merge? Thanks.

From my point of view, it is okay to merge if you are comfortable with the current state. I can try to work in future PRs.