Cleanup tests/test_advection.py fieldset ingestion to use SGRID#2642
Conversation
Needed to relax the tolerance here. Is that acceptable? Might need to revisit the original datasets defined in generic.py to assure the spatial extents are acceptable
|
Failures due to #2643 - once that merges we'll be green. |
VeckoTheGecko
left a comment
VeckoTheGecko
left a comment
There was a problem hiding this comment.
Added some comments for context
| @@ -26,7 +23,7 @@ | |||
| simple_UV_dataset, | |||
| stommel_gyre_dataset, | |||
| ) | |||
| from parcels.interpolators import CGrid_Velocity, XLinear, XLinear_Velocity | |||
| from parcels._datasets.structured.generic import datasets_sgrid | |||
| from parcels.kernels import ( | |||
There was a problem hiding this comment.
The removal of these lower level imports (Field, XGrid, and interpolators) is really nice for this high level set of tests.
| [(None, None), (-0.02, slice(0, 1)), (0.07, slice(None))], | ||
| ids=["no_vertical", "single_w_layer", "full_w"], | ||
| ) | ||
| def test_length1dimensions(u_value, x_slice, v_value, y_slice, w_value, z_slice): |
There was a problem hiding this comment.
For this test I replaced the dataset entirely using one of the existing sgrid ones in datasets_sgrid.
I did need to adjust the velocities and seeding positions though. See
@VeckoTheGecko
Fix spatial extent for test
fba5867
Needed to relax the tolerance here. Is that acceptable? Might need to revisit the original datasets defined in generic.py to assure the spatial extents are acceptable
Should be fine right?
In the other tests, all other dataset changes are just related to now ingesting from SGRID metadata - keeping things the same in the tests
There was a problem hiding this comment.
Yes, I think this is fine
|
|
||
|
|
||
| datasets = { | ||
| datasets_comodo = { |
There was a problem hiding this comment.
To be explicit that these datasets use the (now mostly defunct) COMODO conventions.
Given we're relying more on sgrid I think this renaming is good.
| indexers = {"node_dimension1": x_slice, "node_dimension2": y_slice} | ||
| if w_value: | ||
| indexers.update({"vertical_dimensions_dim1": z_slice}) | ||
| ds = ds.sgrid.isel(indexers) |
There was a problem hiding this comment.
The new .sgrid functionality 🚀
erikvansebille
left a comment
erikvansebille
left a comment
There was a problem hiding this comment.
Excellent, looks all very good to me! Happy to merge on green ticks
|
@fluidnumericsJoe would you say that https://github.com/Parcels-code/Parcels/blob/a228c81ec19d1108df731387755d45633eb9dc5d/tests/test_uxadvection.py would be a good set of integration tests for the unstructured side? Are there any other tests that you think would be helpful for me ? |
2 participants
Description
To confidently overhaul architecture, I would like a set of integration tests that rely on stable API (i.e., the SGRID convention ingestion) that I can use to make sure I'm not breaking anything. Once we finalise the re-organisation, we can go through the rest of the test suite deleting/updating tests.
This set of integration tests is
tests/test_advection.py. At the moment they use old (manual) fieldset creation. This PR updates them to use SGRID instead:Changes:
Checklist
mainfor normal development,v3-supportfor v3 support)AI Disclosure
None used