-
Notifications
You must be signed in to change notification settings - Fork 0
Fix and restucture MOM supergrid logic for OM2 and OM3 #128
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
base: update_om2
Are you sure you want to change the base?
Conversation
a729628 to
3abedd8
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## update_om2 #128 +/- ##
==============================================
+ Coverage 49.28% 50.81% +1.52%
==============================================
Files 18 18
Lines 1469 1604 +135
==============================================
+ Hits 724 815 +91
- Misses 745 789 +44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks @dougiesquire for spending the time helping us resolve the grid issue, it's been incredibly helpful. Now that we understand how to compute different grids during the CMORization process, as well as how to calculate the corner points on a tripolar grid, this will greatly support our future work. However, during the review I still came across a few questions and would like to double check them with you. |
|
Hi @rhaegar325. No worries. Did you want to organise a meeting or can you ask your questions here? |
| # as between the first two rows, for open | ||
| # boundary | ||
| penult_row_rev = np.fliplr(x[-2:-1, :]) | ||
| x_ext = np.append(x, penult_row_rev, axis=0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the comments above, this row should be added to the top of the matrix, but it has been added to the bottom instead. I'm not sure if I’m understanding it correctly, so I just want to double check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this adds to the top as desired?
| penult_col = x_ext[:, -3:-2] | ||
| x_ext = np.append(penult_col, x_ext, axis=1) | ||
| bottom_row = x_ext[:1, :] | ||
| x_ext = np.append(bottom_row, x_ext, axis=0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, it seems that this row should be added to the bottom, but it has been added to the top instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this adds to the bottom as desired?
|
Thanks @dougiesquire. These are the questions I’ve come across so far while doing the review. |
5321402 to
b0bdfee
Compare
rhaegar325
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a quick review, these changes will be incorporated into the CMORisation of the OM2 raw data, and I think it’s ready to be merged. Thanks again @dougiesquire, for your help!
Fixes for #110
I've gotten rid of the
Supergrid_bgridandSupergrid_cgridclasses, which contained a number of bugs. There is now only a singleSupergridclass that extracts all required grid metrics. TheSupergrid.extract_gridmethod now supports arguments for specifying the grid type (Arakawa, cell type and memory mode).I've also restructured the
CMIP6_Ocean_CMORiserclasses accordingly, including fixing a number of bugs.I'm not very familiar with ACCESS-MOPPy as a whole so it would be good if someone could check that these changes will integrate okay.