OSOceanAcoustics / echopype

Enabling interoperability and scalability in ocean sonar data analysis
https://echopype.readthedocs.io/
Apache License 2.0
98 stars 73 forks source link

Support for depth coordinates #259

Closed valentina-s closed 2 years ago

valentina-s commented 3 years ago

Currently echopype's output contain coordinates in units range from sensor. For further analysis, researchers may want to be working with coordinates in watercolumn depth for scientific visualizations and alignment with other variables. It would be nice to have an option to have the water column depth coordinates as part of the object (as opposed to calculating them on the fly every time from the water level). Can the converted data have the option to store depth coordinates? If we have a method coordinate.range_to_depth, we will need to decide if that populates a separate attribute? Is there a danger those are not kept in sync while doing different computations. Thoughts @leewujung, @ngkavin.

leewujung commented 3 years ago

Adding a note here while I review AZFP code:

AZFP has a tilt sensor and the tilt is encoded in the Beam group. When deployed in a mooring, range*cos(tilt) will be the actual depth. (ASL Env Sci provides both a dual cage that's inline with the mooring, or a mooring with 15 deg tilt.)

These should be included when we add function for this range to depth conversion, and in plotting.

leewujung commented 3 years ago

@imranmaj : as we discussed, the calc_range_meter method currently used for calibration can be pulled out the repurpose for this. I think this can be in the form of

echodata = ep.open_raw("some_raw_file.raw")
range_in_meter = echodata.compute_range(...)

Users can just get an xarray DataArray out that they can use in whatever way they want, and we use the same function in the calibration functions as we do right now.

@valentina-s : Do you envision having the depth itself as outputs (as in for EK60/EK80 adding in the water_level parameter)? There is also the intricacy that for AZFP the instruments could have a non negligible tilt that needs to be factor in when converting range to depth (doing a cosine).

@lsetiawan also tagging you here for inputs. :)

leewujung commented 2 years ago

@b-reyes : this is very much related to the addition of the water_level/range_offset in the compute_Sv outputs (#516). Could please you take this on? Thanks!

leewujung commented 2 years ago

Trying to sort this out a bit more, I think we need to decide if we want to allow compute_Sv to directly output a data variable depth, rather than just giving users access to both sonar_range and water_level (or range_offset).

For keeping this general I vote for the latter one (keeping 2 variables).

Note: sonar_range may become echo_range or other names (see #570), and we are renaming water_level to range_offset to comply with the convention (see #516).

Seeking inputs from: @b-reyes @valentina-s @emiliom

b-reyes commented 2 years ago

I personally like the idea of keeping this general i.e. give users access to both sonar_range and range_offset.

leewujung commented 2 years ago

I agree. Let's go with your suggestion!

emiliom commented 2 years ago

Is there a machine friendly way of going from sonar_range, range_offset and other available (meta)data to depth?

leewujung commented 2 years ago

depth = sonar_range + range_offset ?

Do you think we should have a function for this?

leewujung commented 2 years ago

Actually, I take that back, it is not that simple, depending on where the sonar is and its orientation. For shipboard, downward-looking echosounders, it is depth = sonar_range + range_offset

Once the configuration changes, it could be something else. For example, AZFP echosounders are often moored using a cage that has a 15 deg angle to the vertical (gravity) line, so to convert to depth, it will be depth = (sonar_range + range_offset) * cos(15 deg)

So maintaining the two variables separately is more flexible and probably less prone to error (since users have to consciously do the operation.)

emiliom commented 2 years ago

depth = sonar_range + range_offset ?

But only if it's downward looking, right?

Do you think we should have a function for this?

If it were that straightforward in all cases, no. But it's not, is it? eg, aren't the OOI echosounders upward looking (still vertical, though)

emiliom commented 2 years ago

So maintaining the two variables separately is more flexible and probably less prone to error (since users have to consciously do the operation.)

It definitely makes sense to me to keep those variables separate.

But, for what it's worth, that doesn't seem to address the original focus of this issue, unambiguously. Which is fine! I'm just making that distinction.

leewujung commented 2 years ago

Which is fine! I'm just making that distinction.

Haha, no worries.

As we move (develop) up the processing levels, things will have to be along depth at some point. I can see that having Sv data product both ways. I am not sure what may be a good general mechanism, because it is pretty specific to the deployment. Maybe a function with options to select pointing upward or downward, and whether there is a deviation from vertical?

emiliom commented 2 years ago

As we move (develop) up the processing levels, things will have to be along depth at some point. I can see that having Sv data product both ways. I am not sure what may be a good general mechanism, because it is pretty specific to the deployment. Maybe a function with options to select pointing upward or downward, and whether there is a deviation from vertical?

I like where that's going. But like you said, that's for later. BTW, the ability to unambiguously identify the direction the echosounder is pointing is something that's come up in the context of the echogram plotting functions. Gosh, I just remembered that I even created #517 for it!

b-reyes commented 2 years ago

Just to clarify and to close out this issue, we will not be implementing the functionality that allows for the return of the depth (for now). Instead, we will be returning sonar_range (or the name we choose in #570 ) and range_offset. Is this correct?

emiliom commented 2 years ago

@b-reyes that's my understanding, too.

As for whether that closes this issue, @leewujung can decide. The original focus of this issue was somewhat different from what will be implemented. But I think I'd suggest closing the issue and opening a new, similar one (focused on depth) that starts out fresh and picks up some of the discussions in the last couple of comments.

leewujung commented 2 years ago

@b-reyes @emiliom : agreed! I'll close this issue now and put in a new one to capture what we want in the long run.