lkilcher / dolfyn

A library for oceanographic doppler instruments such as Acoustic Doppler Profilers (ADPs, ADCPs) and Acoustic Doppler Velocimeters (ADVs).
BSD 3-Clause "New" or "Revised" License
41 stars 25 forks source link

Allow Velocity or Dataset inputs for API functions #82

Open lkilcher opened 2 years ago

lkilcher commented 2 years ago

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.

lkilcher commented 2 years ago

Still need to:

coveralls commented 2 years ago

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.

Details


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 64.52% -->
Files with Coverage Reduction New Missed Lines %
dolfyn/velocity.py 1 84.98%
<!-- Total: 1 -->
Totals Coverage Status
Change from base Build 1767256983: -0.09%
Covered Lines: 4485
Relevant Lines: 5048

💛 - Coveralls
jmcvey3 commented 2 years ago

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.

lkilcher commented 2 years ago

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.