Fix formatting of cice grid file #18
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18 +/- ##
======================================
Coverage ? 3.38%
======================================
Files ? 20
Lines ? 1241
Branches ? 0
======================================
Hits ? 42
Misses ? 1199
Partials ? 0 ☔ View full report in Codecov by Sentry. |
|
@dougiesquire apologies for the many reviews. please look at #17 first. |
.pre-commit-config.yaml
Outdated
| # See https://pre-commit.com for more information | ||
| # See https://pre-commit.com/hooks.html for more hooks | ||
| repos: | ||
| - repo: local | ||
| hooks: | ||
| - id: black | ||
| name: black | ||
| entry: black | ||
| language: system | ||
| types: [python] |
There was a problem hiding this comment.
Should be fixed now
pyproject.toml
Outdated
| @@ -43,6 +42,7 @@ write_to = "esmgrids/_version.py" | |||
| devel = [ | |||
| "flake8", | |||
| "black", | |||
| "pre-commit" | |||
There was a problem hiding this comment.
Should be fixed now
esmgrids/cice_grid.py
Outdated
|
|
||
| if variant == "cice5-auscom": | ||
| angleT = self._create_2d_nc_var(f, "angleT") | ||
| else: # variant==cice6 |
There was a problem hiding this comment.
I'm assume you chose to do the switching with a string, rather than a binary cice6 = False because you want to allow for the potential for other variants in the future? If so, it might be worth explicitly doing elif variant == "cice6" and producing an error or warning otherwise. Or if it's only ever going to be binary (ie cice5-auscom or cice6) then the API should reflect that.
There was a problem hiding this comment.
Ok. Ill make in a boolean ... its hard to be sure tho. There's 80% chance cice7 would use 'anglet' but maybe it won't, or we find some other fork we need to support that uses a different name.
There was a problem hiding this comment.
Then maybe safest to leave as is but issue an error or warning (e.g. "unrecognised variant, defaulting to cice5-auscom") if it's not a recognised string?
There was a problem hiding this comment.
I want the default to be cice6, but I dont want to change the current interface. So if a variant is provided its set to cice6, if no variant then its cice5-auscom. A bit confusuing maybe. Let me know :)
There was a problem hiding this comment.
Yeah I think that's a bit confusing. I think the default for an unrecognised variant should be the same as the method default. You could just make the method default variant="cice6" and for backwards compatibility someone would have to set variant="cice5-auscom"? That'll be more convenient for us going forward anyway right?
There was a problem hiding this comment.
Yeah that makes sense if you are happy with that? It just breaks back-ward compatibility?
There was a problem hiding this comment.
I think I'm fine with that. As far as we know all existing uses of this package are via a git submodule so won't be a problem.
There was a problem hiding this comment.
Ok cice6 is now by default, cice5-auscom is optional. It raises an error on any other variant that is not cice5-auscom.
This PR fixes two issues:
anglethowever in the cice5-auscom build, we called thisangleT.ni,njfor x and y dimensions. In conjunction with CICE config for CICE6.5 ACCESS-NRI/access-om3-configs#54, this should allow a straight / no-warningsxr.mergeof the grid with history output. Thanks to @aidanheerdegen for the suggestion.Closes #16