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

Further integrating / generalizing wave plotting tooling into trajan #96

Open
jerabaul29 opened this issue Jul 23, 2024 · 1 comment
Open

Comments

@jerabaul29
Copy link
Collaborator

This is originally a summary of what was discussed in #94 and #95 :) .

summary

  • it would be useful to have some form of ds.waves.energy_density_spectrum.plot() and similar functionalities
  • we should decide how general to make this: "1D" vs. "2D" trajectories, i.e. buoy vs model data? plotting of spectra vs. quantities on the trajectories? plots of not only the energy density spectrum but (for other products than the OMB, that would support it), 2D wave spectrum?

copy-pasted previous discussions


  • a few concrete thoughts about the tighter integration to trajan:

    • for now, trajectory plotting is done with ds.traj.plot() and similar. I.e., all trajectory plotting tools are in the .traj attribute. This is a convenient way to group tooling about "trajectories", and it allows to plot unrelated to the underlying details of the ds (which is visible in your code, you can handle quite many different attribute / variable / axis names, formats (1D or 2D), etc). This is nice as it abstracts quite a bit. Would it make sense / would you be opened to doing something similar for the waves? I.e. ds.wave.plot() and similar for plotting waves in general, rather than ds.processed_elevation_spectrum.plot(), which "leaks" a lot about how we define things in the OMB project for now? I am just worried that if we start using different mindsets for different functionalities provided by the same package, this may be confusing for the user. Also, it would be nice to "abstract wave plotting", as trajan is now "abstracting trajectory plotting"
    • if we go for "abstracted wave plotting", I think we would have quite a bit of work discussing / defining / finding out in CF conventions "what wave data is", and making our mind about what we expect to be in the files / how to plot stats vs. 1d spectra vs 2d spectra, how to handle spectra with "1d time" vs "2d time" (i.e. from model vs. buoys as for trajectories I would guess), etc? If so, should I just copy these last 2 points to a new "discussion / issue" so we can take time to discuss this? :)
    • I think if we do this properly for waves, this can also be useful to help extend trajan to more types of data in the future - I guess there is a question of what trajan wants to focus on, and if it aims to plot "all kind of quantities" gathered by buoys or generated from models, not just GPS tracks (and waves) but over any general property like profiles, temperature, etc.? I feel we lack good tooling / standard / examples for "data along trajectories", not just "trajectories" or "waves along trajectories", and trajan may be able to help there. But naturally this is a design choice that you have control over and these are just "my 2 cents", curious to hear your thoughts :) .

Yes, I also find it very tedious to write unnecessarily long names.
On the other hand, the value of the uniqueness and standardness of CF standard name is no great that I have stopped making shorthand aliases, as these will always be ad-hoc and software/person/project-specific, and add volume and complexity to the codebase.
Thus I would in fact favor e.g.
ds.waves.energy_density_spectrum.plot()
over
ds.waves.E.plot()
I am anyway fine with making shorthand aliases if you prefer.


And this is also a very important question: I guess there is a question of what trajan wants to focus on
It is always very tempting to push in a lot of features that one need right now, or for a specific project or task. But in the long run I believe it is healthier for any software to stick to its core tasks. This is why TrajAn was extracted from OpenDrift in the first place, so that OpenDrift can focus on making trajectory simulations, whereas TrajAn can focus on the analysis, and then also applicable to non-OpenDrift data such as buoy measurements.
So there is an important question whether code for handling/analysing/plotting of wave spectra should be within TrajAn, or whether one should rather make a small stand-alone package for this, and then rather let TrajAn use this though some thin wrapper.

Those were my 2 cents, happy to hear any disagreements :-)


And this is also a very important question: I guess there is a question of what trajan wants to focus on It is always very tempting to push in a lot of features that one need right now, or for a specific project or task. But in the long run I believe it is healthier for any software to stick to its core tasks. This is why TrajAn was extracted from OpenDrift in the first place, so that OpenDrift can focus on making trajectory simulations, whereas TrajAn can focus on the analysis, and then also applicable to non-OpenDrift data such as buoy measurements. So there is an important question whether code for handling/analysing/plotting of wave spectra should be within TrajAn, or whether one should rather make a small stand-alone package for this, and then rather let TrajAn use this though some thin wrapper.

Those were my 2 cents, happy to hear any disagreements :-)

I also agree about this, but favor pragmatism: especially if we stick to ds.waves.elevation_density_spectrum or ds.elevation_density_spectrum (which may be provided by cf-xarray) it is easy to separate this into a new package later if it outgrows trajan.

Thus I would in fact favor e.g.
ds.waves.energy_density_spectrum.plot()
over
ds.waves.E.plot()
I am anyway fine with making shorthand aliases if you prefer.

Yes, you are right of course. It is the only solution. Maybe I should just get proper auto-completion in my editor.

@jerabaul29 jerabaul29 changed the title Including wave plotting tooling into trajan Further integrating / generalizing wave plotting tooling into trajan Jul 31, 2024
@gauteh
Copy link
Member

gauteh commented Sep 18, 2024

ds.waves.energy_density_spectrum.plot()
over
ds.waves.E.plot()

This should be handled by: https://cf-xarray.readthedocs.io/en/latest/

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

No branches or pull requests

2 participants