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

update cube_to_target to calculate orographic shape parameter #6876

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

whannah1
Copy link
Contributor

@whannah1 whannah1 commented Jan 7, 2025

This update allows the cube_to_target tool provide the data to support the new orographic drag schemes introduced in PR #6667.

The new output is an optional feature that can be enabled with the "--add-oro-shape" command line flag.

[BFB]

@whannah1 whannah1 added Atmosphere BFB PR leaves answers BFB labels Jan 7, 2025
@whannah1 whannah1 requested review from brhillman and jinboxie January 7, 2025 23:30
subroutine wrtncdf_unstructured(n, terr, sgh, sgh30, &
calc_orographic_shape_params, &
nvar_dirOA, nvar_dirOL, &
orographic_convexity, &
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes require passing the shape parameter arrays even if we are not calculating/writing them. One alternative might be to add a separate function to write these to the output file, but maybe that's not worth the effort. Just an idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, I struggled with this. I think this approach is better than adding a new routine. This is partly because I worry about appending to a file, I've never done that and I'm not sure how to ensure the file is closed before opening it again. Alternatively, I could open/close the file outside the routines that define variables and write the data, but that sounds like a lot more work.

The current approach just passes the flag used to trigger the shape parameter calculation, so it will ignore those extra (unallocated) variables when the shape parameters are not requested.

@whannah1
Copy link
Contributor Author

@jinboxie The current version works to produce a topo file with the shape parameter data. How can we verify that it's being correctly?

@jinboxie
Copy link
Contributor

jinboxie commented Jan 11, 2025

@jinboxie The current version works to produce a topo file with the shape parameter data. How can we verify that it's being correctly?

The easier way is to compare with the current topo data used in BFB test of orodrag.

@whannah1 whannah1 changed the title [WIP] update cube_to_target to calculate orographic shape parameter update cube_to_target to calculate orographic shape parameter Jan 13, 2025
@whannah1
Copy link
Contributor Author

@jinboxie The current version works to produce a topo file with the shape parameter data. How can we verify that it's being correctly?

The easier way is to compare with the current topo data used in BFB test of orodrag.

It might be difficult to ensure BFB with this. I think the actual cube_to_target calculations "should" be BFB, but if I understand correctly, you made the topo file with an outdated version of cube_to_target, and the general/recommended topo workflow has evolved, so I have no idea how to recreate the file you made in a BFB way.

Is there any way we can check if the data look reasonable? Would visual inspection of the shape parameter fields be adequate?

@rljacob
Copy link
Member

rljacob commented Jan 22, 2025

@whannah1 you still need @brhillman or @jinboxie to approve this.

@whannah1
Copy link
Contributor Author

@rljacob actually I need more time on this. Despite my best efforts to not affect the methods it appears that I'm introduced a bug somewhere. I was just plotting up the raw data from a topo file I created against one that @jinboxie created and the fields don't look the same (see figure below).

I'll ping @brhillman for a quick review after I've resolved the problem.

image

@jinboxie
Copy link
Contributor

jinboxie commented Jan 22, 2025

@jinboxie The current version works to produce a topo file with the shape parameter data. How can we verify that it's being correctly?

The easier way is to compare with the current topo data used in BFB test of orodrag.

It might be difficult to ensure BFB with this. I think the actual cube_to_target calculations "should" be BFB, but if I understand correctly, you made the topo file with an outdated version of cube_to_target, and the general/recommended topo workflow has evolved, so I have no idea how to recreate the file you made in a BFB way.

Is there any way we can check if the data look reasonable? Would visual inspection of the shape parameter fields be adequate?

@rljacob actually I need more time on this. Despite my best efforts to not affect the methods it appears that I'm introduced a bug somewhere. I was just plotting up the raw data from a topo file I created against one that @jinboxie created and the fields don't look the same (see figure below).

I'll ping @brhillman for a quick review after I've resolved the problem.

image

Hi Walter,
I was moving and just settled this week at my new home. I think you are currently doing the right comparison by drawing the things out with the old data. I'm not sure how this bug is put in. OA seems to resemble, but other two are weird. I remember OC has some large values at times, so there was a threshold or things in the code to preventing it going too high (I have vague memories on this). OL is weird.
Do you have a package of old code to reproduce the old data to begin with?
Best,
Jinbo

@whannah1
Copy link
Contributor Author

Do you have a package of old code to reproduce the old data to begin with? Best, Jinbo

I have your old code - but it doesn't include some updates to cube_to_target that happened while you were developing the new OD schemes. Merging them via git is too difficult, so I would need to either roll back specific changes to the "new" version or manually update the "old" version to make them directly comparable, both of which would be difficult.

My plan is to keep scanning the new code looking for issues that could explain the discrepancies. I think that's going to be more efficient.

@rljacob
Copy link
Member

rljacob commented Jan 30, 2025

notes: working on getting results consistent.

@whannah1
Copy link
Contributor Author

At long last, I've fixed many bugs and can now reproduce orographic shape parameters that are BFB identical to the baseline as shown in the figure below.

image

There are a few notable caveats attached to how I achieved this.

  • The version of cube_to_target that @jinboxie used for development was pretty far behind the version we have now, and the resulting SGH and PHIS fields are not identical to what we get from the newer cube_to_target. His old version was also used for the topo files being used by the HRv3 effort, so we might need to update those (@xuezhengllnl and @crterai should take note). There's no way I can predict how these differences will affect the HR configuration.

  • The current form of the orographic shape parameter calculations require the land fraction, which has some major effects of the coastal areas. The use of land fraction was removed in a prior cube_to_target update, but it was fairly simple to add it back since the cube3000 data file already includes it. This may cause some pain for super-high-res projects like the California RRM that had to create a new source topography file in the landfrac is not included in those new datasets.

  • In addition to passing the land fraction into the shape parameter routine, to achieve BFB results I also had to restore the manner in which the terrain and SGH fields were modified according to the land fraction. But since the land frac modifications were taken away for a specific reason, I did not want to modify the actual terrain and SGH fields that are used by the model, so I had to make copies of these fields that were then modified by the land fraction. This clutters up the cube_to_target a bit, but I did some additional cosmetic clean up to make up for this.

  • Another problem that caused a lot of pain was that the lat/lon variables were not clearly labelled on whether they were in radians or degrees. I modified the variable names and added some special checks to ensure that this would not cause confusion in the future.

@whannah1
Copy link
Contributor Author

@brhillman can you re-review this?

@jinboxie
Copy link
Contributor

jinboxie commented Jan 30, 2025

At long last, I've fixed many bugs and can now reproduce orographic shape parameters that are BFB identical to the baseline as shown in the figure below.

image

There are a few notable caveats attached to how I achieved this.

  • The version of cube_to_target that @jinboxie used for development was pretty far behind the version we have now, and the resulting SGH and PHIS fields are not identical to what we get from the newer cube_to_target. His old version was also used for the topo files being used by the HRv3 effort, so we might need to update those (@xuezhengllnl and @crterai should take note). There's no way I can predict how these differences will affect the HR configuration.
  • The current form of the orographic shape parameter calculations require the land fraction, which has some major effects of the coastal areas. The use of land fraction was removed in a prior cube_to_target update, but it was fairly simple to add it back since the cube3000 data file already includes it. This may cause some pain for super-high-res projects like the California RRM that had to create a new source topography file in the landfrac is not included in those new datasets.
  • In addition to passing the land fraction into the shape parameter routine, to achieve BFB results I also had to restore the manner in which the terrain and SGH fields were modified according to the land fraction. But since the land frac modifications were taken away for a specific reason, I did not want to modify the actual terrain and SGH fields that are used by the model, so I had to make copies of these fields that were then modified by the land fraction. This clutters up the cube_to_target a bit, but I did some additional cosmetic clean up to make up for this.
  • Another problem that caused a lot of pain was that the lat/lon variables were not clearly labelled on whether they were in radians or degrees. I modified the variable names and added some special checks to ensure that this would not cause confusion in the future.

Hi Walter,
Thanks for the good work @whannah1 . The results are better now. I'd like to note that the PHIS and SGH in the older version is not used. For all the simulations including low-res and high-res simulations, to be consistent with the previous runs, the PHIS and SGH are from the default topo data v3 simulations used (should be generated from the updated cube_to_target you used). I added only the topo parameters documented here into the updated default topo data.
Jinbo

@whannah1
Copy link
Contributor Author

Hi Walter, Thanks for the good work @whannah1 . The results are better now. I'd like to note that the PHIS and SGH in the older version is not used. For all the simulations including low-res and high-res simulations, to be consistent with the previous runs, the PHIS and SGH are from the default topo data v3 simulations used (should be generated from the updated cube_to_target you used). I added only the topo parameters documented here into the updated default topo data. Jinbo

Thanks for that clarification @jinboxie, that was a smart thing to do!

@mt5555
Copy link
Contributor

mt5555 commented Jan 31, 2025

One note: I believe the PR that removed the landfrac stuff was BFB with the old code ( #4384 ). This was one of the reasons we removed landrac - it was not used in anyway.

Speculation: the version @jinboxie used came from NCAR, and the dependence of SGH on landrac is a newer addition done by NCAR that was never in the E3SM version. It's surely a good idea, and if this speculation is correct, we should document that we are bringing in a new change from NCAR. (and consider using it for topo and SGH in the future?)

@whannah1
Copy link
Contributor Author

One note: I believe the PR that removed the landfrac stuff was BFB with the old code ( #4384 ). This was one of the reasons we removed landrac - it was not used in anyway.

Speculation: the version @jinboxie used came from NCAR, and the dependence of SGH on landrac is a newer addition done by NCAR that was never in the E3SM version. It's surely a good idea, and if this speculation is correct, we should document that we are bringing in a new change from NCAR. (and consider using it for topo and SGH in the future?)

If that's true then I can simplify these changes to allow the landfrac to zero out the terr and SGH fields over all-ocean points.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Atmosphere BFB PR leaves answers BFB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants