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

Unstructured reader (FVCOM) #436

Merged
merged 45 commits into from
Dec 1, 2020
Merged

Unstructured reader (FVCOM) #436

merged 45 commits into from
Dec 1, 2020

Conversation

gauteh
Copy link
Member

@gauteh gauteh commented Nov 17, 2020

Rember:

  • Check if wind_from_speed_and_direction works
  • Remove use_block
  • Check if changes to openberg regarding use_block and return_block causes trouble

@gauteh gauteh marked this pull request as draft November 17, 2020 12:56
@knutfrode
Copy link
Collaborator

knutfrode commented Nov 17, 2020

Interesting rearrangement.
Analytical (including constant), structured and unstructured - very logical indeed. Will the unstructured reader cover 26-dimensional bosonic spaces?

Perhaps then the ad-hoc boolean return_block can be removed?

@gauteh
Copy link
Member Author

gauteh commented Nov 17, 2020

Yes, renamed analytical to continuous as that seems to make more sense. Not all models using it are analytical, but they can provide values at continuous input. Unstructured will cover infinite dimensions, but they will mostly be automatically garbage collected by python.

return_block can be removed.

@knutfrode
Copy link
Collaborator

Continuous readers will mostly be continuous in both time and space, though there may be exceptions. E.g. new reader_constant_2d is continuous in time, but discrete/gridded in space.

@gauteh
Copy link
Member Author

gauteh commented Nov 17, 2020

Yes, but it is continuous from the point of view of basemodel - it can take any input value and does its own interpolation (or selects nearest value)?

@gauteh
Copy link
Member Author

gauteh commented Nov 17, 2020

If removing return_block, use_block should maybe also be removed and left to be decided by the readertype (structured, continuous)?

@knutfrode
Copy link
Collaborator

knutfrode commented Nov 18, 2020

On second thought, reader_constant_2d should be a regular structured reader, as it does not do its own interpolation, but returns a block.

So I guess what characterizes analytical or continuous readers is that they do their own interpolation directly onto particle positions/times directly at each call?

Another reader in the borderland is reader_ArtificialOceanEddy, which uses an analytical equation to calculate variables directly at particle positions, but which can also return this on a discrete grid if user has specified use_block = True
I guess we should here chose one of the options, perhaps the former(?)

And then there is the case that reader provides interpolated variables, but with discrete times, such as reader_timeseries
This one presently does it own interpolation in time (actually simply nearest in time).
I guess it makes sense to require that readers which do interpolation in space, should also do interpolation in time (whenever they have a discrete time axis).

@gauteh
Copy link
Member Author

gauteh commented Nov 18, 2020

On second thought, reader_constant_2d should be a regular structured reader, as it does not do its own interpolation, but returns a block.

Yes, exactly.

So I guess what characterizes analytical or continuous readers is that they do their own interpolation directly onto particle positions/times directly at each call?

Yes, indeed. So you can ask for values for a continuous range of arguments. I am open for both names, it is important that we describe these things in the documentation of the class.

Another reader in the borderland is reader_ArtificialOceanEddy, which uses an analytical equation to calculate variables directly at particle positions, but which can also return this on a discrete grid if user has specified use_block = True
I guess we should here chose one of the options, perhaps the former(?)

Is there any advantage in allowing use_block? If not, then the choice is simple.

And then there is the case that reader provides interpolated variables, but with discrete times, such as reader_timeseries
This one presently does it own interpolation in time (actually simply nearest in time).
I guess it makes sense to require that readers which do interpolation in space, should also do interpolation in time (whenever they have a discrete time axis).

That is the current constraint. I think the most natural solution, if following the file/class layout in this PR, is to have StructuredSpatial and StructuredTemporal classes so that they can be composed. Have to think about how exactly that can work, one might have to inherit the other. Maybe we have to go for ContinuousTemporalStructuredSpatial.

@knutfrode
Copy link
Collaborator

I agree that use_block, can be removed. It was probably just a bad idea a long time ago.

@gauteh
Copy link
Member Author

gauteh commented Nov 18, 2020

I agree that use_block, can be removed. It was probably just a bad idea a long time ago.

I don't know about that, but if it is not used anymore it would make things simpler to remove it.

@gauteh
Copy link
Member Author

gauteh commented Nov 18, 2020

Cannot use xarray (yet) because of: pydata/xarray#2233

@gauteh gauteh force-pushed the fvcom branch 3 times, most recently from 7b6d4e6 to acd312f Compare November 19, 2020 19:38
@gauteh
Copy link
Member Author

gauteh commented Nov 23, 2020

See https://github.com/gauteh/opendrift/tree/fvcom-noblock for code removing block and the failing tests.

@gauteh gauteh mentioned this pull request Nov 26, 2020
@gauteh gauteh force-pushed the fvcom branch 3 times, most recently from 22875e7 to b96a3c0 Compare November 27, 2020 13:39
@gauteh gauteh marked this pull request as ready for review November 27, 2020 13:41
@gauteh gauteh changed the title WIP: Unstructured reader (FVCOM) Unstructured reader (FVCOM) Nov 27, 2020
@gauteh
Copy link
Member Author

gauteh commented Nov 27, 2020

Notes on current implementation:

  • Uses nearest neighbor in time and space
  • Slicing on thredds causes too many opendap-requests, see if .diagonal() is better for remote sources. Will probably always be an issue.
  • animations does not work, because basemodel probably need to use get_variables_interpolated_xy(..)
  • Seems to work for local files.
  • Slightly unsure about mapping sigma depth to z depth.

@gauteh
Copy link
Member Author

gauteh commented Nov 30, 2020

Converting the FVCOM file from Netcdf3 to Netcdf4 doubles the speed for local files, and will probably make it easier for opendap servers as well.

@knutfrode
Copy link
Collaborator

After the two last commits, I expect all examples to complete.

…e block is True (verified against master with block = True as well)
@gauteh
Copy link
Member Author

gauteh commented Nov 30, 2020 via email

@gauteh gauteh merged commit 55f3b3d into OpenDrift:master Dec 1, 2020
@gauteh
Copy link
Member Author

gauteh commented Dec 1, 2020 via email

@gauteh gauteh deleted the fvcom branch September 21, 2021 17:20
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.

2 participants