dolfyn
dolfyn copied to clipboard
Allow Velocity or Dataset inputs for API functions
The idea here is that API functions (such as rotate2) can handle (take as input and return the matching dtype) either xarray.Dataset or velocity.Velocity objects. i.e.:
dsi = dolfyn.read('some raw file in inst coords')
vdsi = dsi.velds
vdse = dolfyn.rotate2(vdsi, 'earth', inplace=False) # an velocity.Velocity is returned (matches input vdsi)
dse = dolfyn.rotate2(dsi, 'earth', inplace=False) # an xarray.Dataset is returned (matches input dsi)
This seems like a really nice feature, however it is problematic when doing something like this:
_dse = dsi.velds.rotate2('earth', inplace=False)
In this case it is unclear whether _dse should be a Velocity object or a Dataset (currently it's a Dataset). The obvious solution to this would be to remove the option for inplace=False from the velocity.Velocity methods and always do inplace=True. However, I think inplace=False is sometimes nice. On the other hand, this can easily be accomplished with the function.
Still need to:
- document the behavior in the API docs
- add some tests
- delete todo-branch.md
Pull Request Test Coverage Report for Build 1767259736
Warning: This coverage report may be inaccurate.
This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
- For more information on this, see Tracking coverage changes with pull request builds.
- To avoid this issue with future PRs, see these Recommended CI Configurations.
- For a quick fix, rebase this PR at GitHub. Your next report should be accurate.
Details
- 20 of 31 (64.52%) changed or added relevant lines in 4 files are covered.
- 1 unchanged line in 1 file lost coverage.
- Overall coverage decreased (-0.09%) to 88.847%
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
|---|---|---|---|
| dolfyn/velocity.py | 3 | 4 | 75.0% |
| dolfyn/tools/misc.py | 7 | 17 | 41.18% |
| <!-- | Total: | 20 | 31 |
| Files with Coverage Reduction | New Missed Lines | % |
|---|---|---|
| dolfyn/velocity.py | 1 | 84.98% |
| <!-- | Total: | 1 |
| Totals | |
|---|---|
| Change from base Build 1767256983: | -0.09% |
| Covered Lines: | 4485 |
| Relevant Lines: | 5048 |
💛 - Coveralls
Is this the original view you had for dolfyn? I like the simplified view, but if dolfyn is going to be completely incorporated into mhkit, I have a feeling it may be best to leave this as a reference object to work off of in the rest of the mhkit api. I fear it'll intimidate users to have another layer of dolfyn functionality to learn.
For the sake of discussion, I'm not sure how difficult this would be to change the functions. I didn't totally infuse xarray functionality into the source code, leaving it essentially "matlab-like" and numpy-based, so I'm sure it can be refactored a little to handle this class type in addition to data-sets/arrays.
The idea here is to have two different views onto the data. One that is just an xarray.Dataset, and another that has a little more of an 'adcp/adv data analysts perspective' and convenience methods (e.g., .save, .rotate2, etc.). But creating both views does come with a WHOLE LOT of effort in terms of maintaining/documenting/testing the code. So, as much as I like the idea of having both views supported, I'm currently inclined not to merge this in. However, I'm going to leave this open for the time being, and might think on it some more.