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

part of cam6_3_134: Rename SE dycore time_mod to dyn_time_mod #904

Conversation

jimmielin
Copy link
Member

@jimmielin jimmielin commented Oct 16, 2023

This commit renames time_mod in the SE dycore to dyn_time_mod in order to avoid naming conflicts with building with GEOS-Chem (with the SE dycore enabled).

All USE statements updated; zero differences in run output are expected.

This is being submitted as a separate PR than #484 as it is a zero-diff update of file names and might need discussion that is separate from the GEOS-Chem changes to CAM code.

On the GEOS-Chem side we intend to rename our utilities in the GEOS-Chem code with a prefix to avoid future instances of naming conflicts as well. This will be discussed by the GEOS-Chem support team. Copying @lizziel.

Thanks!

This commit renames time_mod in the SE dycore to dyn_time_mod in order to avoid naming conflicts with building with GEOS-Chem.

All USE statements updated; zero differences in run output are expected.
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.

Thanks for implementing these changes! I was just wondering if you could make the new module name a little more specific to avoid this same issue possibly happening again in the future.

@@ -1,4 +1,4 @@
module time_mod
module dyn_time_mod
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like dyn_time_mod is still generic enough that we could run into name collisions again in the future. Could you possibly change this to be se_dyn_time_mod instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! Now renamed to se_dyn_time_mod.

Signed-off-by: Haipeng Lin <hplin@seas.harvard.edu>
@jimmielin jimmielin force-pushed the hplin/rename_time_mod_conflict branch from c9d226c to fc2cf9b Compare October 18, 2023 23:36
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.

Thanks @jimmielin! These changes now look good to me, so I have opened this up for full review.

@peverwhee peverwhee assigned nusbaume and unassigned fvitt Oct 26, 2023
@peverwhee
Copy link
Collaborator

@PeterHjortLauritzen Can you review this when you get a moment?

@peverwhee peverwhee changed the title Rename SE dycore time_mod to dyn_time_mod part of cam6_3_134: Rename SE dycore time_mod to dyn_time_mod Oct 26, 2023
Copy link
Collaborator

@PeterHjortLauritzen PeterHjortLauritzen 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 to me!

@nusbaume
Copy link
Collaborator

nusbaume commented Nov 1, 2023

Not sure why the closing script didn't catch this, but these code changes were brought in with PR #891, and so this PR can be closed.

@nusbaume nusbaume closed this Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

6 participants