-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
depends on light hermes #49
base: main
Are you sure you want to change the base?
Conversation
@@ -7578,7 +7579,14 @@ subroutine Coldstart(gc, import, export, clock, rc) | |||
bk_is_missing = .true. | |||
endif | |||
|
|||
if (ak_is_missing .or. bk_is_missing) call set_eta(km, ls, ptop, pint, AK, BK) | |||
if (ak_is_missing .or. bk_is_missing) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This duplicates logic above. It should be moved into an internal procedure and then just called from the two locations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interfaces are different. I am not sure how to avoid the logic. Since it is already moved to init(), it is not a big issue.
@@ -326,7 +330,11 @@ program interp_restarts | |||
|
|||
allocate ( r8_ak(npz+1) ) | |||
allocate ( r8_bk(npz+1) ) | |||
call set_eta(npz,ks,ptop,pint,r8_ak,r8_bk) | |||
if (trim(eta_rc_file) == 'None') then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and duplicated again here ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly little things, but the read during the run method bothers me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry - missed that there were two grid comps that needed the change.
Still might be worth refactoring to a shared procedure, but ... harder now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice.
If there were any changes, I might ask that you modify the name of the internal state. Conventionally these are named after the component itself, though I think your name is much clearer. So please leave it as is, but be prepared to switch it if anyone asks.
This merge requires the merge of GMAO_Shared branch feature/wjiang/#149-add_write_eta which provide the module shared_topo_remap