syntax for calibration calls

[leewujung]

I wanted to get some inputs on how the syntax should be for calibration calls with our new EchoData object in the upcoming v0.5.0. I think this will set the pattern for how we add more data processing/analysis functions later (especially since we're going to have breaking changes in the API calls (even though we can make it backward compatible), so want to be a little careful.

Let's use calibration to obain Sv as an example.
To create an EchoData object, one would do

import echopype as ep
echodata = EchoData('path_to_a_converted_file')

To calibrate the raw data in this file, we can do

# Option 1
Sv = ep.calibrate.get_Sv(echodata)

Or we can group calibrate to under process and do

# Option 2
Sv = ep.process.get_Sv(echodata)

It boils down to if we consider calibration to be part of "processing", but I feel more modularization would be better in the long run, since there are so many ways to "process" data. It almost seems that one should avoid that module name altogether. For example, a processing could be school detection that can be called by:

schools = ep.detection.school(echodata)

instead of

schools = ep.process.detect_schools(echodata)

Right now I structured the code to do Option 1.

@emiliom @valentina-s @lsetiawan @ngkavin @imranmaj : any suggestions welcome! Thanks!

[lsetiawan]

My 2cent: I think calibrate should be separate than process since in my mind this is the unifying function that converts the various data types to a uniform model before passing to other process functions.

[emiliom]

Process (and model before that) was always an umbrella, wasn't it? I assume you found it convenient and coherent with users' expectations for common processing steps.

It boils down to if we consider calibration to be part of "processing", but I feel more modularization would be better in the long run, since there are so many ways to "process" data. It almost seems that one should avoid that module name altogether.

I don't know about others, but as far as I'm concerned you are the expert here 😉. I don't think there is a right or wrong answer about whether calibration is "processing". Process could go. Right now (according to the docs) there are just 3 Process methods: calibrate, remove_noise and get_MVBS. If you don't anticipate that number to grow much, I think getting rid of Process may have more benefits than drawbacks.

On an additional note, it looks like you're shifting from the pattern of Process having methods (eg, calibrate) to EchoData objects acted on by functions. ie:

# From this 
ed = Process('path_to_a_converted_file')
ed.calibrate()

# To this
echodata = EchoData('path_to_a_converted_file')
Sv = ep.calibrate.get_Sv(echodata)

I'm not saying one is better than the other. But it is a change. The new pattern is also a departure from the way Convert operates, which is also based on object methods.

Also, while previously "get Sv" was effectively what calibrate did, now you've made it explicit. I assume calibrate will have other methods as well? Otherwise it seems redundant.

[leewujung]

If you don't anticipate that number to grow much, I think getting rid of Process may have more benefits than drawbacks.

The number of processing functions will grow for sure. Hopefully soon... I really want to get out of this revising Convert forever situation!

I assume calibrate will have other methods as well? Otherwise it seems redundant.

Yes. For example, I was thinking to add calibrate.get_single_target_TS(...) which will allow people to compute frequency-dependent target strength from single targets from EK80 data, within a supplied time/range region. That's why I think it'll be useful to group similar functionalities under different modules, like the above example detect.school(echodata).

The new pattern is also a departure from the way Convert operates, which is also based on object methods.

Yes, I did spend some time thinking about this, so great that you brought it up so we can discuss that also. I actually went through a few iterations (from function calls to OO calls and then back) and arrived at this current form. Changes are easy to make right now and not later, so let's figure out what's the best to do and fix it. :)

From the perspective of adding more processing functions in the long run, it seems easier to expand when we simply treat EchoData as objects to be explicitly operated on by different functions, instead of the previous Process class where the echo data are implicitly operated on within the object once it is initialized. We can obviously design OO type of calls for this, but I wonder if it is actually making things more complicated than necessary. Perhaps it is just what users get used to do. For example, xr.merge([ds1, ds2]) is not an OO call, but Datasets are initialized via OO type call like ds = xr.Dataset('xyz.zarr') and many functions are invoked by object calls like ds.assign_coords(...). 🤯

[leewujung]

Alright, folks, after the fun discussions last week, here's what we decided:

• We will use compute_Sv instead of get_Sv to avoid confusion with get/set concepts
• We will remove the specialized EchoData object
  • this object was originally designed to handle things like combining multiple files and checking compatibility between the raw and processed data, etc. We can do those checks under the hood in the functions.
  • For the need to open multiple groups to be able to perform operations such as compute_Sv, it seems that it is a widely requested feature that we can just do those under the hood also for now and change when it is supported by xarray.
  • This means that we won't be able to do chained operations, like echodata.calibrate.compute_Sv().compute_MVBS() (this actually looks like a bad idea :P ), but will likely be less confusing.
• We will write the processing operations as function calls, instead of OO style calls, that operates on xarray Datasets (or files containing data that could be read into Datasets) and returns xarray Datasets.
  • This should make it straightforward for users with basic knowledge of pandas/xarray operations to be able to use the functions.
  • This also maintains the type of input-output consistency in pandas/xarray
• For the cost of opening files, we can always have a compute=True/False option so that we can delay the costly operations to when the daskworkers are actually doing the job.

Anything I missed here?