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

Feat/obs interpolated position #104

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jerabaul29
Copy link
Collaborator

Following recurrent user requests, this PR:

  • adds a robust function to interpolate messy / "with holes" time series, e.g. gnss positions; the idea is that real world data are messy and there may be NaNs, NaTs, holes, etc. So these are checked quite carefully, and a "robust best attempt" is done: interpolate as NaN if there are no close data points, as "nearest" if there is close data points only on one side in time, as "linear" if there is close data points on both sides in time
  • performs interpolation of the gnss lat and lon to the times of the wave measurements in the omb reader, and include these in the xr produced by the omb reader
  • document this in the in-depth omb example

@jerabaul29
Copy link
Collaborator Author

Looks like the CI test pipeline fails because of something else than this code for now:

Error: This request has been automatically failed because it uses a deprecated version of actions/upload-artifact: v2. Learn more: https://github.blog/changelog/2024-02-13-deprecation-notice-v1-and-v2-of-the-artifact-actions/

@jerabaul29
Copy link
Collaborator Author

(@gauteh are you familiar with this CI error? looks like this is something standard? something on trajan CI in general that could be updated? how do you want to proceed? If you push a fix to main I can rebase on it :) )

@jerabaul29
Copy link
Collaborator Author

Fixes #103 .

@gauteh
Copy link
Member

gauteh commented Sep 18, 2024

Hi Jean,

I'm looking at this now and thinking about how to do this so that we get a consistent set of features and methods in TrajAn. It seems that this functionality is very similar to that of gridtime, and that gridtime perhaps is a special case of interpolating observational data to a time coordinate.

The other question is how much post-processed data to include in these files. The goal should not be a one-liner in trajan in itself. You have to call that one-liner from some python-script anyway, so when you generate data for a particular end-user or purpose it is better to add the specific adaptations there as another line or two and keep the default in trajan as basic as possible. That will make the methods easier to compose into blocks for different purposes. We see in OpenDrift that adding a lot of particular functions or arguments to functions is a never-ending game, the next use case is slightly different and a new function or argument is needed. Better to have the user write a few lines, than one very specific one.

@knutfrode
Copy link
Contributor

Hi!
I dit not study in very much detail, but also get the impression that there is probably quite a bit overlap between existing gridtime and the new interpolate_variable_to_newtimes
And so I share Gautes concern that we might here fall into the trap of Duplication, which is the Root of all Evil:
https://dev.to/ivangavlik/dont-repeat-yourself-practical-advice-at-code-level-1mem
The problem is then that it is very hard to improve on this later, when there will be many internal and external cross-dependencies.

Thus small and well defined methods with very specific and clear task and input-output is desired, rather than larger methods which are very convenient ("oneliner") for a specific task, but just slightly unusable for similar tasks.
The existing gridtime was also made relatively quickly, so it is probably not ideal either.
So maybe we should sit down one day, i.e. a hackaton trying to outline a best strategy as clearly as possible?

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