-
Notifications
You must be signed in to change notification settings - Fork 19
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
Creating Vertical Diffusion Solver #133
Creating Vertical Diffusion Solver #133
Conversation
Pull ZM 0.5 timestep removal into main
Replace original, locally-developed license with Apache 2.0 license.
Bring in development ESCOMP#108 (TUV-x) ESCOMP#112 (tropopause_find) to main
Bring in new directory structure from development
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.
Thanks @mwaxmonsky! This looks good, I just had a few minor comments. The most major one is just to check the name of the to-be-ccpp(-)ized folder since it's not in ESCOMP/atmospheric_physics now.
@@ -0,0 +1,152 @@ | |||
module coords_1d |
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.
Just confirming the directory is to be named to-be-ccpp-ized
or to-be-ccppized
(https://github.com/cacraigucar/atmospheric_physics/tree/atmos_phys_zmcleanup3/to_be_ccppized) since I've seen two variants around!
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.
Good catch @jimmielin!
@nusbaume, @cacraigucar, @peverwhee, we might have discussed this previously so my apologies for not taking better notes but do we have a preferred layout/naming convention for these non-ccppized files? Particularly with coords1d and linear_1d_operators being helper files (which might be needed by other files later) and the high level interface in the solver file.
I think having them at separate layers might be best for organization but not too sure on the naming conventions.
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.
After discussing this with @jimmielin and @cacraigucar I believe we decided for the to_be_ccppized
directory to be completely flat. This was to essentially make that directory ugly on purpose so that future users/developers remain motivated to get everything CCPP-ized. Of course if anyone disagrees with this plan please let me know!
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.
Flattened!
to-be-ccpp-ized/utilities/vertical_diffusion/vertical_diffusion_solver.F90
Outdated
Show resolved
Hide resolved
to-be-ccpp-ized/utilities/vertical_diffusion/vertical_diffusion_solver.F90
Outdated
Show resolved
Hide resolved
to-be-ccpp-ized/utilities/vertical_diffusion/vertical_diffusion_solver.F90
Outdated
Show resolved
Hide resolved
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.
Thanks @mwaxmonsky for addressing my comments, I left the to-be-ccpp(-)ized
and contiguous
comments unresolved as there's some unfinished discussions on those. Once these are confirmed it should be good to go, I will approve the PR 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.
Looks good! Just had some small code cleanup requests.
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.
Everything looks great to me now. Thanks @mwaxmonsky!
Originator(s): @mwaxmonsky
Summary (include the keyword ['closes', 'fixes', 'resolves'] and issue number):
Describe any changes made to the namelist: N\A
List all files eliminated and why: N\A
List all files added and what they do:
A to_be_ccppized/coords_1d.F90
A to_be_ccppized/linear_1d_operators.F90
A to_be_ccppized/vertical_diffusion_solver.F90
List all existing files that have been modified, and describe the changes: N\A
(Helpful git command:
git diff --name-status development...<your_branch_name>
)List any test failures: None
Is this a science-changing update? New physics package, algorithm change, tuning changes, etc?
N\A