Skip to content

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

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.

Sign up for GitHub

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

Merged
erikvansebille merged 4 commits into from
Feb 10, 2025
Merged

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

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
19185aa
Support for Fields in multiple files without a `time` dimension
erikvansebille Jan 29, 2025
6fdafcd
Merge branch 'main' into fix_croco_multifiles_csw
erikvansebille Feb 4, 2025
4fd16ec
Fixing case where timestamps explicitly given
erikvansebille Feb 4, 2025
16c249c
Merge branch 'fix_croco_multifiles_csw' of https://github.com/OceanPa…
erikvansebille Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions parcels/field.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -684,12 +684,17 @@
if grid is None:
# Concatenate time variable to determine overall dimension
# across multiple files
time, time_origin, timeslices, dataFiles = cls._collect_timeslices(
timestamps, data_filenames, _grid_fb_class, dimensions, indices, netcdf_engine
)
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

@VeckoTheGecko VeckoTheGecko Jan 29, 2025

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

@erikvansebille erikvansebille Jan 29, 2025

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

@VeckoTheGecko VeckoTheGecko Jan 29, 2025

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

@erikvansebille erikvansebille Feb 4, 2025

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

time, time_origin, timeslices, dataFiles = cls._collect_timeslices(
timestamps, data_filenames, _grid_fb_class, dimensions, indices, netcdf_engine

Check warning on line 689 in parcels/field.py

View check run for this annotation

Codecov / codecov/patch

parcels/field.py#L687-L689 

Added lines #L687 - L689 were not covered by tests
)
grid = Grid.create_grid(lon, lat, depth, time, time_origin=time_origin, mesh=mesh)
grid.timeslices = timeslices
kwargs["dataFiles"] = dataFiles

Check warning on line 693 in parcels/field.py

View check run for this annotation

Codecov / codecov/patch

parcels/field.py#L691-L693 

Added lines #L691 - L693 were not covered by tests
else: # e.g. for the CROCO CS_w field, see https://github.com/OceanParcels/Parcels/issues/1831
grid = Grid.create_grid(lon, lat, depth, np.array([0.0]), time_origin=TimeConverter(0.0), mesh=mesh)
grid.timeslices = [[0]]
data_filenames = [data_filenames[0]]

Check warning on line 697 in parcels/field.py

View check run for this annotation

Codecov / codecov/patch

parcels/field.py#L695-L697 

Added lines #L695 - L697 were not covered by tests
elif grid is not None and ("dataFiles" not in kwargs or kwargs["dataFiles"] is None):
# ==== means: the field has a shared grid, but may have different data files, so we need to collect the
# ==== correct file time series again.
Expand Down
Loading