Skip to content

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

Copy link
Contributor

@VeckoTheGecko VeckoTheGecko left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, I thought I had already approved

@erikvansebille erikvansebille merged commit 429d3ae into main Feb 10, 2025
15 of 16 checks passed
@erikvansebille erikvansebille deleted the fix_croco_multifiles_csw branch February 10, 2025 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

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

3 participants