Some code cleanup #51
Conversation
regional_mom6/regional_mom6.py
Outdated
| `λ` (numpy.array): All longitude points on the supergrid. | ||
| `φ` (numpy.array): All latitude points on the supergrid. |
There was a problem hiding this comment.
Don't add Markdown to arguments docstrings like this, they are already recognised as variables from the context.
There was a problem hiding this comment.
OK -- didn't know!
Also what is "the super grid"? And if λ, φ are on the super grid then why we divide by 2 further down?
There was a problem hiding this comment.
The supergrid is the set of all cell centres and corners, so it kind of looks like a double-resolution tracer grid. I expect they're divided by 2 to get the normal resolution grid.
There was a problem hiding this comment.
oh I see!
so is the λ, φ inputs of this function the "supergrid" but then the output is the normal grid? then dλ = np.diff(λ) * 2 (and not / 2), right?
@ashjbarnes can elaborate perhaps?
There was a problem hiding this comment.
Hmm yeah, division does sound strange in that case.
There was a problem hiding this comment.
Sorry for such a late reply! I've totally abandoned this while I've been away. Was this resolved in the end? This code was all copied from the NWA people in a rush with the intention of rewriting later.
There was a problem hiding this comment.
There is no reason to apologize!
But, what is NWA?
There was a problem hiding this comment.
NWA is the North West Atlantic experiment that the NOAA people have done. This is where some of the code has been borrowed from, esp for the grid gen https://github.com/jsimkins2/nwa25
| return angle | ||
|
|
||
|
|
||
| # Borrowed from grid tools (GFDL) | ||
| def quad_area(lat, lon): | ||
| """Returns area of spherical quad (bounded by great arcs).""" | ||
| """Returns area of spherical quadrilaterals (bounded by great arcs).""" |
There was a problem hiding this comment.
Guess this should get a proper docstring?
There was a problem hiding this comment.
Yes! I'm trying to understand first what it does.. there is some discussion in #39. :)
There was a problem hiding this comment.
Ok so on first pass this code fails when running the demo notebooks. Firstly there was a typo (y written instead of phi in line 347) but on fixing it I then get this error:
To test I cloned the demo notebook, checked that it still worked using the main branch then switched to the PR branch.
Basically looks like the new function modifies the size of what used to be x/y so the broadcast no longer works. I don't really have my head around what it's doing so didn't want to just fiddle with it to fix broadcast issue!
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #51 +/- ##
==========================================
- Coverage 15.61% 15.42% -0.20%
==========================================
Files 2 2
Lines 429 428 -1
==========================================
- Hits 67 66 -1
Misses 362 362
☔ View full report in Codecov by Sentry. |
navidcy
added
documentation 📔
regional_mom6/regional_mom6.py
Outdated
| dx = np.broadcast_to( | ||
| np.pi * R * np.cos(np.pi * y / 180) * 0.5 * res / 180, | ||
| (x.shape[0] - 1, y.shape[0]), | ||
| R * np.cos(np.deg2rad(y)) * np.deg2rad(np.diff(λ)) / 2, |
There was a problem hiding this comment.
This gained a np.diff? Does it change anything?
There was a problem hiding this comment.
was part of my questions to @ashjbarnes; I didn't understand why we weren't doing it previously.
the array should be the same but now the method also allows for non-uniform spacing in longitude... probably not necessary... I would definitely like to see a horizontal grid comparison using main and this PR before merging.
There was a problem hiding this comment.
Ah yeah, just making sure we have a comment at the right code location :)
There was a problem hiding this comment.
before this was just np.diff(λ) -> res which was equal to x[1]-x[0]
This was mostly done in an attempt to understand what the functions are doing.
There was somewhere a definition of 1 degree in radians and then used to multiply all lat, lon values. I presume that's simply to convert degrees to radians. I changed to use
np.deg2radwhich is self-explanatory.Also I relaxed the restriction that longitudes needed to be equidistant and that thedx = x[1] - x[0]. Nowdx = np.diff(x); will this create issues?I haven't tested anything. @ashjbarnes have a look and please test? Supposedly the code should not be different so perhaps we can compare a grid constructed on
mainand the same grid on this PR to ensure that nothing broke?