Flow-Dependent, Cross-Timescale Model Diagnostic POD #127
Conversation
drewmresnick
requested review from
jkrasting,
tsjackson-noaa and
wrongkindofdoctor
as code owners
jkrasting
changed the title
|
That would be me. Instructions here:
#125
…On Thu, Feb 4, 2021 at 5:00 PM drewmresnick ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In diagnostics/flow_dep_diags/doc/flow_dep_diags.rst
<#127 (comment)>
:
> @@ -0,0 +1,136 @@
+.. This is a comment in RestructuredText format (two periods and a space).
@jkrasting <https://github.com/jkrasting> could you point me to who I
should contact regarding submission of the data for this POD?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#127 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHLJVQBJTIGRU4OHVVN7PMTS5MYJJANCNFSM4W5X4LTQ>
.
|
|
This pull request introduces 5 alerts when merging ca56b80 into 333fac2 - view on LGTM.com new alerts:
|
…-diagnostics into feature/flow_dep_diags
|
This pull request introduces 7 alerts when merging 89b8d69 into 578f3d6 - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
@drewmresnick I've requested some final minor corrections. Also, remove the unused variables and module import caught by LGTM.
| @@ -157,6 +156,22 @@ | |||
| "units": "W m-2", | |||
| "ndim": 3 | |||
| }, | |||
| // Variables for flow_dep_diag module: | |||
There was a problem hiding this comment.
We need to add in the companion fields to the GFDL and CMIP fieldlists
There was a problem hiding this comment.
@jkrasting Is this then not something I need to worry about?
There was a problem hiding this comment.
@drewmresnick This is a todo for the framework team, so one of us will take care of it.
There was a problem hiding this comment.
Thanks for bringing this to my attention. Unfortunately constructions of the from variable_at_XX_mbar aren't CF standard names [*].
The way variables on pressure levels are intended to be specified in the settings file is described in the documentation: the level is specified separately with a scalar_coordinate attribute, which refers to a vertical coordinate axis defined in the dimensions section of the file. By spelling out what's meant in more detail like this, it e.g. enables the framework to extract levels from 3D data on the fly in a fully units-aware way.
For more examples of how this is set up, look at the settings file for the Wheeler-Kiladis diagnostic.
[*]: I haven't been able to find a concise reference for this: the upshot is that standard names are meant to identify physical quantities, independently of how that quantity is sampled in space or time (but there are exceptions to this, such as air_pressure_at_sea_level, which deal with physical rather than mathematical boundaries). Variables on pressure levels in the CMIP6 data request don't use different standard names in this way (example), and pressure levels aren't one of the cases covered in construction of derived standard names.
|
|
||
| #the diagnostic assumes your grid has a longitudinal range -180,180 | ||
| #Shift your grid from 0,360 using the following lines | ||
| ds.coords['lon'] = (ds.coords['lon'] + 180) % 360 - 180 #shift the values |
There was a problem hiding this comment.
Test condition before applying the shift.
There was a problem hiding this comment.
I do not think it needs to test for a condition because if the values are between -180,180 the correct grid units will be retained using this calculation. I will add comments to explain this.
| @@ -0,0 +1,103 @@ | |||
| name: MDTF_flow_dep_diag | |||
There was a problem hiding this comment.
This YAML is overly prescriptive. At a minimum, remove the build codes. Consider just listing the main top-level packages rather than every dependency.
There was a problem hiding this comment.
@jkrasting Is there a way to automatically generate the yaml file with only top-level packages. I know at least for removing the build codes I can use the --no-builds flag.
There was a problem hiding this comment.
@drewmresnick you can try using conda env export --from-history > flow_dep_diag.yml per this SO reply. However, the post states that this will not include package versions. Otherwise, you'll have to modify the file manually, as conda does not seem to have this exact capability.
|
Hi, @drewmresnick. It's been a while since you've updated your PR, so I'm just checking to make sure that things are going okay with the development. Let me know if you have any ?'s, or need help with anything. |
Submission of Dummy PR
This PR only includes a partially complete diagnostics documentation file. The rest of the documentation and additional files are still being compiled.