xnetcdf and 2-d lat/lon coordinates - #963
Conversation
sadielbartholomew
left a comment
There was a problem hiding this comment.
Looks great overall. Some minor comments as usual - see in-line and the more general comments below. There are some merge confilcts to address also, but once all has been considered, happy for you to merge.
-
Though the
umreadC library files & headers have been deleted, theumread_libdirectory still remains on this branch and it would be good to tidy by removing it. I think to do that we'd have to remove reference to it (e.g. to the built.oobject files locations) from the.gitignore, commit that, and then clean andrmdiror similar. Could do this as follow-on work, but if you're up to include it in the PR that would sort it to nicely blitzumreadin this PR.
(Possibly the directory remains locally for me due to my having the built object files in the sub-directoryc-lib, but I think the.gitignorewould also prevent it from going. -
There is a
pyprojwarning emerging from thetest_Field_2d_create_latlon_coordinates_albers_equal_areawhich could be important:test_Field_2d_create_latlon_coordinates_albers_equal_area (__main__.LatLon2dTest.test_Field_2d_create_latlon_coordinates_albers_equal_area) Test albers_equal_area. ... /home/slb93/miniconda3/envs/cf-env-314/lib/python3.14/site-packages/pyproj/crs/crs.py:1295: UserWarning: You will likely lose important projection information when converting to a PROJ string from another format. See: https://proj.org/faq.html#what-is-the-best-format-for-describing-coordinate-reference-systems proj = self._crs.to_proj4(version=version)
-
pyprojshould be listed in the package files and docs as an optional dependency, at least - but (thinking about the imminent release) can you confirm it isn't intended as a compulsory one (it isn't fully clear without some investigation how widespread the grid mapping logic is to the codebase)?
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
Hopefully a13e32c will do that
It is optional: 6aca0fd
I see that this is wholly benign - we're only calling |
|
Hi Sadie. Fantastic. All comments resolved, or responded to. I'm just re-running the whole test suite ... and all tests pass :) |
Nice. We might need a final
Aha sorry I should have been more specific and said it is missing in an 'extras_require' category in the Lines 240 to 263 in f7e655d Ideally you can put it there in an appropriate place, else I can do it for the release.
Ah good to know, thanks for investigating and preventing it. All good there then. Doing a final sanity check then you can merge! |
sadielbartholomew
left a comment
There was a problem hiding this comment.
All feedback addressed, thanks, though feel free to tackle the minor tweaks mentioned in my latest here: #963 (comment), if not I can do those alongside the release checklist, before merging. Good to merge either way.
|
|
Fixes #961 and #962