Skip to content

Conversation

@andrew-s28
Copy link
Contributor

This is a small change that changes a ValueError (raised when initializing a field which does not have a time dimension from multiple files, e.g., Cs_w used in FieldSet.from_croco()) into a FieldSetWarning. This enables the support for such fields implemented in #1835.

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.

Thanks for the pr! Just a small edit - I think due to the nature of this issue it would be good to also link to it in the error message

parcels/field.py Outdated

if len(data_filenames) > 1 and "time" not in dimensions and timestamps is None:
raise RuntimeError("Multiple files given but no time dimension specified")
warnings.warn("Multiple files given but no time dimension specified", FieldSetWarning, stacklevel=2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
warnings.warn("Multiple files given but no time dimension specified", FieldSetWarning, stacklevel=2)
warnings.warn("Multiple files given but no time dimension specified. See https://github.com/OceanParcels/Parcels/issues/1831 for more info.", FieldSetWarning, stacklevel=2)

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, on second thought, perhaps linkint to the issue would be confusing - I'm not sure in which scenarios people would be expected to see the error message ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'm not sure linking to the issue is necessary here. Prior to the implementation of the CROCO model support, which relies on parameters like Cs_w and H that don't change in time and was the original source of issues, the error would have caught someone using multiple velocity files (perhaps each intended to each be a different time), but without a time dimension in the files themselves, which does seems like it would be a rare edge case.

A potential alternative that would leave that error in place would be to add on to the if statement a condition that excludes only the fields names (self.name?) used by the CROCO advection (H, Cs_w, and zeta). But I thought just keeping it simple and swapping to a warning is also fine, since this is all an admittedly niche use case.

@erikvansebille erikvansebille merged commit c42ca55 into Parcels-code:main Feb 27, 2025
16 checks passed
@github-project-automation github-project-automation bot moved this from Backlog to Done in Parcels development Feb 27, 2025
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.

3 participants