Skip to content
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

Support for Fields in multiple files without a time dimension #1835

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

erikvansebille
Copy link
Member

This PR fixes #1831 by treating Fields that are in multiple files but don't have a time dimension explicitly. An example is the Cs_w Field needed in FieldSet.from_croco()

parcels/field.py Outdated
grid = Grid.create_grid(lon, lat, depth, time, time_origin=time_origin, mesh=mesh)
grid.timeslices = timeslices
kwargs["dataFiles"] = dataFiles
if "time" in dimensions:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the code before didn't error in _collect_timeslices (only in the array assignment later), are there times (i.e., in other setups) where "time" isn't in dimensions, but the field isn't CS_w? Hence adding this clause could cause something else to break? Just want to understand a bit more before merging since I'm not super familiar with this code

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I understand your concern. This part of the code is so bloated that it's not easy to keep the oversight. But given that we have failing CI, something is probably going wrong. I'll look into how I can find a different, more robust way to fix this issue

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking forward to using xarray 😁

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the code again, and I now do think this is robust code.

The functionality of the part of the code that I changed is to create a list timeslices of the time associated with each snapshot of a Field in a netcdf file. But if a Field doesn't have a time dimension, this operation does not make sense.

So I'm pretty confident that this PR will have no major unintended consequences; it's just that other Fields without a time dimension never reach this part of the code so it's not an issue for these.

I could dig much deeper what happens to these fields then, but I suggest we leave that for a major refactor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

parcels.FieldSet.from_croco() has issues with Fields that have multiple times
2 participants