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

Merge in DoD onyx mods #3100

Closed

Conversation

apcraig
Copy link
Contributor

@apcraig apcraig commented Jul 31, 2019

Port to onyx.
Updates default pe layout for ARRM 60to10 and 60to6 configurations.
Updates default mpas-ocean namelist for ARRM 60to10 and 60to6 configurations

@@ -8446,4 +8446,78 @@
</pes>
</mach>
</grid>
<grid name="a%T62.+oi%oARRM60to10|a%TL319.+oi%oARRM60to10">
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to add these changes for a specific machine. Also, @apcraig: we should talk about the improvements you experienced with these settings (we can do this by email).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I will take care of that.

@@ -440,14 +442,20 @@
<config_use_activeTracers>.true.</config_use_activeTracers>
<config_use_activeTracers_surface_bulk_forcing>.true.</config_use_activeTracers_surface_bulk_forcing>
<config_use_activeTracers_surface_restoring>.false.</config_use_activeTracers_surface_restoring>
<config_use_activeTracers_surface_restoring ocn_grid="oARRM60to10">.true.</config_use_activeTracers_surface_restoring>
<config_use_activeTracers_surface_restoring ocn_grid="oARRM60to6">.true.</config_use_activeTracers_surface_restoring>
Copy link
Contributor

Choose a reason for hiding this comment

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

these should not be default because we only do restoring in G-cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will back out all of the changes to the mpaso namelist_defaults. I think that is more consistent with the overall strategy of using user_nl_mpaso to set things up and the benefit is the namelist defaults file remains consistent with master.

<config_use_activeTracers_interior_restoring>.false.</config_use_activeTracers_interior_restoring>
<config_use_activeTracers_exponential_decay>.false.</config_use_activeTracers_exponential_decay>
<config_use_activeTracers_idealAge_forcing>.false.</config_use_activeTracers_idealAge_forcing>
<config_use_activeTracers_ttd_forcing>.false.</config_use_activeTracers_ttd_forcing>
<config_use_surface_salinity_monthly_restoring>.false.</config_use_surface_salinity_monthly_restoring>
<config_use_surface_salinity_monthly_restoring ocn_grid="oARRM60to10">.true.</config_use_surface_salinity_monthly_restoring>
<config_use_surface_salinity_monthly_restoring ocn_grid="oARRM60to6">.true.</config_use_surface_salinity_monthly_restoring>
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same comment as above.

@@ -28,7 +28,8 @@ add_default($nl, 'config_full_abort_write');
# Namelist group: decomposition #
#################################

add_default($nl, 'config_block_decomp_file_prefix', 'val'=>"'${DIN_LOC_ROOT}/ice/mpas-cice/${ICE_GRID}/mpas-cice.graph.info.${date_stamp}.part.'");
#add_default($nl, 'config_block_decomp_file_prefix', 'val'=>"'${DIN_LOC_ROOT}/ice/mpas-cice/${ICE_GRID}/mpas-cice.graph.info.${date_stamp}.part.'");
add_default($nl, 'config_block_decomp_file_prefix', 'val'=>"'${DIN_LOC_ROOT}/ice/mpas-cice/${ICE_GRID}/mpas-seaice.graph.v2.${date_stamp}.part.'");
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please explain this change? Shouldn't the default be the graph.info file since we do not know for sure if the v2 version is available for all meshes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have reviewed this and will remove it. I just grepped for graph in the mpas-seaice stuff and made changes in all places that seemed appropriate. I have now looked at the implementation and agree this should be left unchanged.

@apcraig
Copy link
Contributor Author

apcraig commented Aug 1, 2019

@rljacob , @jgfouca . This is a PR from one branch to another. I don't think we need E3SM in general involved. @milenaveneziani and I are discussing and taking care of it. I hope we are not breaking any E3SM process by using PRs. It's cleaner and easier that pushing directly as it provides an opportunity for multiple people to review easier. Thanks.

@jgfouca jgfouca assigned milenaveneziani and unassigned jgfouca Aug 1, 2019
@jgfouca
Copy link
Member

jgfouca commented Aug 1, 2019

@apcraig , I didn't notice that. I have reassigned to @milenaveneziani

@rljacob
Copy link
Member

rljacob commented Aug 1, 2019

I didn't notice that either. We really don't want these kind of PRs appearing in our backlog. You should do them on a fork instead.

@apcraig
Copy link
Contributor Author

apcraig commented Aug 1, 2019

@milenaveneziani , what do you think about the process? Should we move the branch to a branch on a fork? Or should we avoid PRs in the future and just push to the E3SM branch as needed? I think PRs are a nice way to merge that provides some additional features over just pushing. But willing to use whatever process you prefer.

@milenaveneziani
Copy link
Contributor

I think we should move it to a fork. I can push the branch to your fork and we can start again there?

@milenaveneziani
Copy link
Contributor

@apcraig: I have pushed this branch to my fork. I will close this now.

jgfouca pushed a commit that referenced this pull request Nov 7, 2024
…un-eamxx-workflows

Always run eamxx workflows on PRs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants