Skip to content
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

Merged
merged 19 commits into from
Oct 24, 2024

Conversation

mwaxmonsky
Copy link
Collaborator

@mwaxmonsky mwaxmonsky commented Oct 14, 2024

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

  • Adds initial set of files to support solve step of vertical diffusion. Top level utilities files are copied directly from CAM and will be removed from CAM when integrating.

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

@mwaxmonsky mwaxmonsky self-assigned this Oct 14, 2024
@jimmielin jimmielin self-requested a review October 14, 2024 21:11
@mwaxmonsky mwaxmonsky marked this pull request as ready for review October 14, 2024 21:13
Copy link
Member

@jimmielin jimmielin left a 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
Copy link
Member

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!

Copy link
Collaborator Author

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.

Copy link
Collaborator

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!

@jimmielin @cacraigucar @peverwhee

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flattened!

Copy link
Member

@jimmielin jimmielin left a 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.

Copy link
Collaborator

@nusbaume nusbaume left a 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.

to_be_ccppized/vertical_diffusion_solver.F90 Outdated Show resolved Hide resolved
to_be_ccppized/vertical_diffusion_solver.F90 Outdated Show resolved Hide resolved
to_be_ccppized/vertical_diffusion_solver.F90 Outdated Show resolved Hide resolved
to_be_ccppized/vertical_diffusion_solver.F90 Outdated Show resolved Hide resolved
Copy link
Collaborator

@nusbaume nusbaume left a 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!

@mwaxmonsky mwaxmonsky merged commit c6ed2ca into ESCOMP:development Oct 24, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants