Update fieldset ingestion to use convert modules#40
Open
VeckoTheGecko wants to merge 11 commits intomainfrom
Open
Update fieldset ingestion to use convert modules#40VeckoTheGecko wants to merge 11 commits intomainfrom
VeckoTheGecko wants to merge 11 commits intomainfrom
Conversation
Contributor
Author
I'm going to get started setting the groundwork on this - keen to discuss if either of you have ideas so this can be further refined :) |
Contributor
Author
Joe mentions that we have a convert function for this |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
OK - I've gone ahead and updated the ingestion code here so that its inline with Parcels-code/Parcels#2549 . We are closer to having a working benchmark suite, but unfortunately we're not there yet. Hence I propose that we go ahead and merge this anyway as it gets us closer to the end goal.
MOI error
Currently ingestion works, but we get an error during the execution itself (note this PR now closes #33, as ingestion works and we now have a different error).
FESOM error
Here we get an error on the selection of the interpolator - this is a bug upstream in Parcels (this dataset has dims
('time', 'nz1', 'elem', 'nod2', 'nz')but_select_uxinterpolatordoesn't expect these dimension namings isn't able to determine the right interpolators. AFAICT this problem was always here for this dataset. Let me know what you think @fluidnumerics-joe ).pixi run setup-data pixi run asv run --bench 'fesom2.*'Future work
Better testing
I'm finding it quite difficult to debug all of this since it's working using heavy datasets, and the iteration loop using
asv runis very frustrating (e.g., run benchmark, find error (that I can't easily open using pdb since that has poor integration withasv), recreate the error using a normal python script, realise the bug is in Parcels, etc).At the moment we have the following (which can be thought of as a pyramid - from the top to the most foundational):
There is, however, the possibility of a layer in between:
open_cdl? pydata/xarray#6269 andds.to_dict(data=False)is close to what we need, but excludes coordiantes)This intermediate layer is hinted at in our "Participating in the issue tracker: 'Parcels doesn't work with my data'" doc page section, but I think can be formalised and also extended to the coordinates (as those are also quite important). Regarding implementation, we can create a separate repo to host these small files (similar to https://github.com/Parcels-code/parcels-data )
This intermediate layer has the following benefits:
Keen to hear your thoughts @erikvansebille .
Better cataloguing
From https://discourse.pangeo.io/t/data-pipelining-and-cataloging-best-practices-using-intake-xarray-to-transform-and-combine-data-metadata/5550/6 , I think we can streamline how we ingest data (by using Intake 2 in combination with the
convertmodule or in combination withuxarray). Honestly, this is a low priority - I'm happy with what we have at the moment.The important this from my POV is the "Better testing" above as that will flag any errors with our convert module.