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

Fixes for zlev problem in #1459 #1469

Merged
merged 8 commits into from
May 7, 2017
Merged

Fixes for zlev problem in #1459 #1469

merged 8 commits into from
May 7, 2017

Conversation

jedwards4b
Copy link
Contributor

The level specifier for atm and lnd grids was being ignored.
Test suite: scripts_regression_tests.py, SMS.ne30z60_ne30.F1850
Test baseline:
Test namelist changes:
Test status: bit for bit
Fixes #1459

User interface changes?:

Code review:

Copy link

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

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

I have pushed a proposed fix to the branch for review.
I think the regular expression for levmatch should also be updated on line 237 (original version) or 241 (my version). Thoughts?

if component_gridname == 'atm' and atmnlev is not None:
lname += "z" + atmnlev
elif component_gridname == 'lnd' and lndnlev is not None:
lname += "z" + lndnlev
Copy link

Choose a reason for hiding this comment

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

I think we may need some error checking here. I tried an A compset with a grid alias of ne30z32_ne30z14 and ended up with LND_GRID="nullz14" in env_build.xml
I'm not sure this damages anything but I don't think this is intended.
Maybe modifying the land levmatch:
levmatch = re.match(r"(.*_)([^_]+)z(\d+)(_[^m].*)$", name)
and add an additional check to each if block 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.

I think that this regex changes is broken if the mask is not present isn't it?

@mvertens mvertens changed the title Mvertens/fixzlev Fixes for zlev problem in #1459 May 5, 2017
if component_gridname == 'atm' and atmnlev is not None:
lname += "z" + atmnlev
elif component_gridname == 'lnd' and lndnlev is not None:
lname += "z" + lndnlev
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 think that this regex changes is broken if the mask is not present isn't it?

@@ -57,7 +57,7 @@ def get_grid_info(self, name, compset):
name = levmatch.group(1)+levmatch.group(3)

#mechanism to specify lnd levels
levmatch = re.match(r"(.*_)([^_]+)z(\d+)(.*)$", name)
levmatch = re.match(r"(.*_)([^_]+)z(\d+)(_[^m].*)$", name)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is group 4 always expected to be present?

Copy link

Choose a reason for hiding this comment

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

No but at the moment, there is no way to specify land levels if it is not present. For example:
ne30z192_ne30 always specifies atmosphere levels. If you want to specify land levels, you need a tri-grid short name which means group 4 will be present.

@@ -205,9 +205,13 @@ def _read_config_grids_v2(self, name, compset, atmnlev, lndnlev):
if model_grid[component_gridname] is not None:
lname += model_grid[component_gridname]
if component_gridname == 'atm' and atmnlev is not None:
lname += "z" + atmnlev
if re.search(r"a%null", lname) is None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You don't need a regex here
if not "a%null" in lname:
should do it.

Copy link

Choose a reason for hiding this comment

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

Doh, will fix.

elif component_gridname == 'lnd' and lndnlev is not None:
lname += "z" + lndnlev
if re.search(r"l%null", lname) is None:
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 here, don't use regex if you don't need to

Copy link

Choose a reason for hiding this comment

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

ditto

@@ -234,7 +238,7 @@ def _get_domains_v2(self, component_grids):
levmatch = re.match(r"([^_]+)z(\d+)(.*)$", grid_name)
if levmatch:
grid_name_nonlev = levmatch.group(1)+levmatch.group(3)
levmatch = re.match(r"(.*_)([^_]+)z(\d+)(.*)$", grid_name)
levmatch = re.match(r"(.*_)([^_]+)z(\d+)(_[^m].*)$", grid_name)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we reorganize so that the regex expression only appears in the code once?

Copy link

Choose a reason for hiding this comment

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

Is that the royal we?
I'll look into it.

@mvertens
Copy link
Contributor

mvertens commented May 5, 2017

@JEdwards @gold2718 - there is a subtle problem with the naming convention we have. If you do not have a trigrid (atm/land on different grids) there is no way to distinguish if the zNN is meant for the atmosphere or for the land.

@jedwards4b
Copy link
Contributor Author

@mvertens that doesn't sound right - f09z30_f09 looks different than f09_f09z30

@gold2718
Copy link

gold2718 commented May 5, 2017

but the second f09 is for the ocean/ice, not for the land. Do we really want to add land levels to the ocean grid name?

@jedwards4b
Copy link
Contributor Author

Oh - you got me there - would it work then to specify a tri-grid like this for this special case?
f09_f09z30_f09

@gold2718
Copy link

gold2718 commented May 5, 2017

That is the idea (and what I'm testing). That is why the (anti-)mask test is in the land regex.

@gold2718
Copy link

gold2718 commented May 5, 2017

Okay, changes are in. Reran scripts_regression_tests on Hobart and code checker on Yellowstone.

@jedwards4b jedwards4b self-assigned this May 5, 2017
@jedwards4b
Copy link
Contributor Author

If you'll update the status I'll go ahead and merge.

@gold2718 gold2718 removed their assignment May 5, 2017
@gold2718 gold2718 requested a review from mvertens May 5, 2017 17:20
@mvertens mvertens assigned mvertens and unassigned jedwards4b May 7, 2017
@mvertens mvertens merged commit 653dd29 into master May 7, 2017
@mvertens mvertens deleted the mvertens/fixzlev branch May 9, 2017 19:37
jgfouca added a commit that referenced this pull request Jun 2, 2017
Add tests for ne30_oECv3_ICG.A_WCYCL1850S

Replace three integration tests that were using
ne30_oEC.A_WCYCL2000
with
ne30_oECv3_ICG.A_WCYCL1850S
The above is the config/compset actually used by the coupled group.

[BFB]

* origin/rljacob/coupled/test-spinups:
  Introduced tests for ne30_oECv3_ICG.A_WCYCL1850S
jgfouca added a commit that referenced this pull request Feb 23, 2018
Add tests for ne30_oECv3_ICG.A_WCYCL1850S

Replace three integration tests that were using
ne30_oEC.A_WCYCL2000
with
ne30_oECv3_ICG.A_WCYCL1850S
The above is the config/compset actually used by the coupled group.

[BFB]

* origin/rljacob/coupled/test-spinups:
  Introduced tests for ne30_oECv3_ICG.A_WCYCL1850S
jgfouca added a commit that referenced this pull request Mar 13, 2018
Add tests for ne30_oECv3_ICG.A_WCYCL1850S

Replace three integration tests that were using
ne30_oEC.A_WCYCL2000
with
ne30_oECv3_ICG.A_WCYCL1850S
The above is the config/compset actually used by the coupled group.

[BFB]

* origin/rljacob/coupled/test-spinups:
  Introduced tests for ne30_oECv3_ICG.A_WCYCL1850S
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.

3 participants