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

If area units on source grid file are in sq deg, convert to sq radians #2022

Merged
merged 2 commits into from
Nov 3, 2017

Conversation

billsacks
Copy link
Member

@billsacks billsacks commented Nov 2, 2017

This just applies to source grid files in the scrip format.

Without this fix, areas were assumed to be in sq radians, which led to
mapping weights that were off by about a factor of 3000 if areas were
actually in sq degrees.

Test suite: Tested runoff_map tool with a source file with units in sq
degrees; confirmed that area_a and S values now have the
right order of magnitude. Also tested with a source file
with units in sq radians, and confirmed that the resulting
mapping file is the same as before.
Test baseline: n/a
Test namelist changes: none
Test status: bit for bit

Fixes #2018

User interface changes?: none

Update gh-pages html (Y/N)?: N

Code review:

This just applies to source grid files in the scrip format.

Without this fix, areas were assumed to be in sq radians, which led to
mapping weights that were off by about a factor of 3000 if areas were
actually in sq degrees.
@billsacks
Copy link
Member Author

@alperaltuntas - @mnlevy1981 suggested that we start adding you as a reviewer for runoff mapping-related changes, so I have done that here. I also went ahead and made you the assignee. Let me know if you have questions about the process of being an assignee, because it looks like this may be the first cime PR for which you're the assignee.

Copy link
Contributor

@mnlevy1981 mnlevy1981 left a comment

Choose a reason for hiding this comment

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

What happens if units is not an attribute in the netCDF file? (Should you check rcode of the nf_get_att_text call before checking the value of units?) I would be okay with treating a missing unit attribute as "square radians", since it seems like that matches previous behavior.

Also give a more helpful message if grid_area has unrecognized units
@billsacks
Copy link
Member Author

Good catch. I have just pushed a commit that aborts if no units are found. I'm more comfortable with this conservative approach rather than assuming units of square radians.

@mnlevy1981
Copy link
Contributor

@billsacks -- I'm okay with aborting if no units are found, but I'd like to wait until cheyenne is back up and test this by re-generating r05 and r01 to gx3v7 maps (something I think we need to do anyway because of #2010); I don't like introducing new exit criteria without verifying that our current examples don't trigger it. If it turns out that we need to add units to an existing SCRIP grid file, then we should also update the corresponding nml files that come bundled with this tool.

@billsacks
Copy link
Member Author

@mnlevy1981 The r05, r01 and rx1 mappings do not enter the block of code modified in this PR, because they use 'rtm'-formatted files (for r05 and r01) or 'obs'-formatted files (for rx1).

The two map_wr50a_* template namelist files do use 'scrip'-formatted inputs. I'm not sure what these are for: I don't see anything referring to wr50 in either CESM or ACME's config_grids file.

As an aside: In looking into this, I noticed that (if I'm reading the code correctly) incorrect areas and mapping weights will currently be used if you try to generate mappings with the r01 grid, because 'rtm'-formatted files use this block of code, which assumes a 0.5 deg grid:

         map%area_a(n) = 0.5 * 0.5 * cos(lat*DEGtoRAD) * DEGtoRAD * DEGtoRAD

Copy link
Contributor

@mnlevy1981 mnlevy1981 left a comment

Choose a reason for hiding this comment

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

@billsacks I was 99% sure this block of code was not run with our current runoff maps, but thanks for verifying.

Is the r01 grid used at all? I have a vague memory of noticing the hard-coded 0.5 in the line you mentioned, but I didn't do anything about it because I figured it was only encountered with the r05 grid. Another possible fix would be to add rtm_cell_width_deg or something similar to the namelist (with default value of 0.5) and instructions to set it properly when the runoff grid is rtm-formatted.

@ekluzek
Copy link
Contributor

ekluzek commented Nov 3, 2017

@mnlevy1981 I'm pretty sure we had problems with the r01 grid and hence haven't used it. There is a 8th degree grid for MOSART, but that hasn't been a standard grid that we run with either. Collin Z. used it in some high res. simulations that he did, but there were problems with it. So I don't know that we have any working high resolution ROF grids.

But, I do think there is a need for this for high resolution work. So we might want to check with scientists on the need for it.

@billsacks
Copy link
Member Author

Is the r01 grid used at all? I have a vague memory of noticing the hard-coded 0.5 in the line you mentioned, but I didn't do anything about it because I figured it was only encountered with the r05 grid. Another possible fix would be to add rtm_cell_width_deg or something similar to the namelist (with default value of 0.5) and instructions to set it properly when the runoff grid is rtm-formatted.

We have old mapping files from r01 to gx1v6 map_r01_to_gx1v6_120711.nc but not to gx1v7 or any other grid.

It feels like the best solution would be to require rtm-formatted files to have the necessary metadata on them. That would require making a new version of the r05 (and r01) file with this metadata added. I'm also okay with your proposed namelist option, though - as long as the default is some 'unset' value so that it is required to be set explicitly. I think this should be deferred to a separate issue (as opposed to folding it into this PR).

@mnlevy1981
Copy link
Contributor

It feels like the best solution would be to require rtm-formatted files to have the necessary metadata on them. That would require making a new version of the r05 (and r01) file with this metadata added.

Oh, that's a clever solution

I think this should be deferred to a separate issue (as opposed to folding it into this PR

I agree

@alperaltuntas alperaltuntas merged commit 28f74aa into ESMCI:master Nov 3, 2017
@billsacks billsacks deleted the runoff_convert_sq_degrees branch November 3, 2017 17:33
jgfouca pushed a commit that referenced this pull request Feb 23, 2018
Because the CPU processors in Titan's login, sevice and compute nodes
are different, the "aprun" command is always required even if the
model is compiled in mpi-serial. This PR is to add the "aprun" command
before "acme.exe" in the batch script on Titan when the model is compiled
in mpi-serial. It will fix the issue #2022

[BFB] - Bit-For-Bit
jgfouca pushed a commit that referenced this pull request Feb 23, 2018
…#2023)

Because the CPU processors in Titan's login, service and compute nodes
are different, the "aprun" command is always required even if the
model is compiled in mpi-serial. This PR is to add the "aprun" command
before "acme.exe" in the batch script on Titan when the model is compiled
in mpi-serial. It will fix the issue #2022

[BFB] - Bit-For-Bit
jgfouca pushed a commit that referenced this pull request Mar 13, 2018
Because the CPU processors in Titan's login, sevice and compute nodes
are different, the "aprun" command is always required even if the
model is compiled in mpi-serial. This PR is to add the "aprun" command
before "acme.exe" in the batch script on Titan when the model is compiled
in mpi-serial. It will fix the issue #2022

[BFB] - Bit-For-Bit
jgfouca pushed a commit that referenced this pull request Mar 13, 2018
…#2023)

Because the CPU processors in Titan's login, service and compute nodes
are different, the "aprun" command is always required even if the
model is compiled in mpi-serial. This PR is to add the "aprun" command
before "acme.exe" in the batch script on Titan when the model is compiled
in mpi-serial. It will fix the issue #2022

[BFB] - Bit-For-Bit
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.

Runoff mapping generates incorrect weights for scrip grid files with area in square degrees
5 participants