Skip to content

Conversation

@poplarShift
Copy link
Contributor

…synchronously. In practice, this allows you to lower the model timestep without penalty in terms of simulation time, since you won't need to wait for the next timestep to load as you enter it. (with an appropriately low timestep, that is)

Supersedes #1752

…synchronously. In practice, this allows you to lower the model timestep without penalty in terms of simulation time, since you won't need to wait for the next timestep to load as you enter it. (with an appropriately low timestep, that is)
def _configure_fields_to_read_from_fvcom(self):
"""
FVCOM stores fields with different long_names and variable names than done internally in
opendrift. Furthermore, we want to be able to advect using either 2d or 3d fields.
Copy link
Collaborator

Choose a reason for hiding this comment

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

OpenDrift follows CF-convention, and so should FVCOM.
https://cfconventions.org/Data/cf-standard-names/current/build/cf-standard-name-table.html

variable names and long names are mostly irrelevant and ignored - as these are user/institution/model-specific and not standardised. standard_name attribute is the only thing that matters.

@knutfrode
Copy link
Collaborator

knutfrode commented Jan 6, 2026

(Tests seem to be failing due to a file that you forgot to include.)

Very interesting work!
How much faster is it with the asynchronous reading based on your experience?
And is there a specific reason why this mechanism is not included in the other FVCOM reader that is based on Xarray?

If it works well and robustly - it would be of interest to generalise this concept to other (any) readers.

But, in line with comments to #1756:
With this and #1756 we have two readers specific for FVCOM in addition to the generic unstructured reader.

However, there should not be a need for any FVCOM-specific-readers at all - we should ideally have just a single unstructured reader adhering to the UGRID conventions - and which is also planned.
So a much better solution would be to have model-specific "preprocessors" that would fix any non-compliance, and forward a CF and UGRID-compliant dataset to the generic unstructured reader which may then stay clean - and will be the same for any unstructured models. Benefits will be large - e.g. implementing the asynchroneous feature in the generic reader would then favor all unstructured models, and not only FVCOM as of here. And it will be much easier to maintain and further develop the code for the future - in contrast to now with e.g. a lot of FVCOM-specific mess in three different readers.

However, as this clean-up is not likely to happen in the next few months, I am fine with including this reader in the meantime. But we should then add the missing file and ensure that all tests are passing.

@knutfrode
Copy link
Collaborator

knutfrode commented Jan 6, 2026

To illustrate one possible solution for a future more generic and cleaner usage:

r = reader_netCDF_CF_unstructured(<files/URLS>, preprocessor="auto")

where preprocessor could be:

  • None to use the raw dataset as opened with Xarray
  • 'auto' (default) run through all available preprocessors and apply if they detect that the dataset is of a specific type, e.g. FVCOM
  • fvcom' (e.g.) to use the fvcom preprocessor explicitly.

a preprocessor could be a single module in a subfolder named "preprocessors", and which opens and transforms a dataset to become both CF-compliant (in all cases) and UGRID compliant (in the case of unstructured modeles).

E.g. a FVCOM preprocessor would add the missing standard_name attributes, and add and properly name the necessary UGRID geolocation attributes/variables.
You could then also have different preprocessors for different FVCOM-versions (including in-house-specific setups), or (ideally, if possible) let the preprocessor handle all variants.

An alternative to the above, would be that the reader of any non-compliant datasets would be a thin wrapper that performs the compatibility transformation of the dataset, before calling the generic unstructured reader. Then you could still have readers named e.g. reader_fvcom.py, with pseudocode:

- open dataset/URLs
- add missing standard_names and geolocation variables
- call contructor of reader_unstructured

@HvardE
Copy link

HvardE commented Jan 9, 2026

(Tests seem to be failing due to a file that you forgot to include.)

Very interesting work! How much faster is it with the asynchronous reading based on your experience?


It's not much faster, I mostly use it to afford a lower timestep. The total simulation time is still limited by read time for the next timestep.

But in the best case scenario, if reading one timestep takes 5s and you iterate while it's loading, you essentially save 5s pr block of data loaded to memory throughout the simulation compared to using the same timestep and stopping to load at each block. In practice the saving is less, since you often have to turn down the timestep in opendrift so that you don't need to stop and wait for the next block.

I've noticed that I have less particles hitting the shore when using the async reader due to the lower timestep, which I suppose is the most significant change for our use cases.

@HvardE
Copy link

HvardE commented Jan 9, 2026

And is there a specific reason why this mechanism is not included in the other FVCOM reader that is based on Xarray?


The only reason why it's not in the xarray reader is that we've worked on the readers separately. I would also like to add the vertical interpolation scheme to the xarray reader at some point.

@knutfrode
Copy link
Collaborator

The only reason why it's not in the xarray reader is that we've worked on the readers separately. I would also like to add the vertical interpolation scheme to the xarray reader at some point.

Ok, that is understandable. But then a common goal is that these two FVCOM readers should be merged in the future - and with the ultimate goal of a generic unstructured reader that works for all unstructured models.

@HvardE
Copy link

HvardE commented Jan 9, 2026

The only reason why it's not in the xarray reader is that we've worked on the readers separately. I would also like to add the vertical interpolation scheme to the xarray reader at some point.

Ok, that is understandable. But then a common goal is that these two FVCOM readers should be merged in the future - and with the ultimate goal of a generic unstructured reader that works for all unstructured models.

Yep, and I think we can do that within a few months - so to keep things clearer, I think don't need to merge this reader into the repository, but rather update the xarray one with the methods proposed in this reader (y)

@knutfrode
Copy link
Collaborator

Yes, I agree it is better to not load too much "work in progess" into the main repository.
But then we can let this PR stay unmerged, still allowing others to peek into the stuff to come.

Btw, for testing it would be useful to have some "standard" or "reference" FVCOM output files to test on. But I am not ware of any such, and understand the different versions of FVCOM can have slightly different output.
If you are aware of any such datasets, we could make some FVCOM-specific unit-tests to ensure the reader will work with all variants.

@HvardE
Copy link

HvardE commented Jan 9, 2026

Yes, I agree it is better to not load too much "work in progess" into the main repository. But then we can let this PR stay unmerged, still allowing others to peek into the stuff to come.

Btw, for testing it would be useful to have some "standard" or "reference" FVCOM output files to test on. But I am not ware of any such, and understand the different versions of FVCOM can have slightly different output. If you are aware of any such datasets, we could make some FVCOM-specific unit-tests to ensure the reader will work with all variants.

Ok, sounds like we have a plan :) I talked briefly with @poplarShift about subsetting data from one of our models that can be used for testing. We just have to find time for it, we will come back to you once we have a plan!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants