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

cam_cesm2_1_rel_57: Transport CL and Br when mid-atmos chem is used in cesm2.1 #435

Merged
merged 9 commits into from
Jan 5, 2023

Conversation

fvitt
Copy link

@fvitt fvitt commented Sep 10, 2021

Remove CL and BR from the not-transported lists in middle-atmosphere chemistry mechanisms.

Addresses #314 for CESM2.1

@fvitt fvitt added the answer changing answer changing tag label Sep 10, 2021
@fvitt fvitt self-assigned this Sep 10, 2021
Copy link
Collaborator

@cacraigucar cacraigucar left a comment

Choose a reason for hiding this comment

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

All files are pp files, so easy approval

@@ -937,7 +937,7 @@ if [ -n "${submit_script_cb}" ]; then
case $hostname in
# cheyenne
chey* | r* )
batch_queue_submit='qsub '
batch_queue_submit='qsub -V '
Copy link
Author

Choose a reason for hiding this comment

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

@cacraigucar I need to include the -V for cheyenne. Otherwise, the stand-alone cam tests don't work correctly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious - what changed since you last made a CESM2_1 branch tag back in September, do you know?

Copy link
Author

Choose a reason for hiding this comment

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

I needed the same changes for the cam_cesm2_1_rel_56 also. I just did not include the changes to test_driver.sh in the tag.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In that case, I wholeheartedly approve this change. I'm not surprised that there are changes like this needed for the branch which is so old.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@cacraigucar I need to include the -V for cheyenne. Otherwise, the stand-alone cam tests don't work correctly.

To me, this sounds like an error in the submit script. Any environmental variables from test driver that are not resolved in the script (i.e., the environmental variable is still there) should be evaluated before being written to the script. For example, if the script depends on variable, FOO:

export FOO=bar

then we should not see $FOO or ${FOO} in the script, we should see bar.
I think it is a lot more robust to do this rather than depend on the environment. Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

@gold2718 You make a good point. Given that this is for a machine that does not have much life left and for a branch that has very little development activity, I prefer to go with my simple fix. I don't want to spend any more time on this.
@cacraigucar @nusbaume Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it is not fixed now, I suggest opening an issue to get this fixed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that we are under a HARD deadline before the PGI compiler goes "poof", I would vote to just make this an issue and not fix it here. We can then fix it whenever we replace the PGI tests with GNU, or if/when we port this branch to Derecho (whichever comes first).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree to move ahead with this and @fvitt can open an issue detailing what he saw.

@gold2718 gold2718 removed their request for review January 5, 2023 22:03
@fvitt fvitt merged commit 6904fe4 into ESCOMP:cam_cesm2_1_rel Jan 5, 2023
@cacraigucar cacraigucar changed the title Transport CL and Br when mid-atmos chem is used in cesm2.1 cam_cesm2_1_rel_57: Transport CL and Br when mid-atmos chem is used in cesm2.1 Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
answer changing answer changing tag
Projects
Status: Tag
Development

Successfully merging this pull request may close these issues.

4 participants