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

Enable use of data from EchoData object in consolidate.add_depth #1205

Closed
wants to merge 7 commits into from

Conversation

emiliom
Copy link
Collaborator

@emiliom emiliom commented Oct 28, 2023

Overhaul consolidate.add_depth to enable using data from EchoData object. Addresses #790. See the echopype documentation for reference on several of the variables.

Rotations are performed using scipy.spatial.transform.Rotation.

Notes and caveats:

  • See the TODOs in the code.
  • If both an EchoData object and external data (as scalar arguments) are passed, only data from the EchoData object is used.
  • I'm seeing too many nans in the resulting depth variable for the sample EK80 file, so I suspect something isn't working properly.
  • AZFP tilt_x/y variables are not handled. Only standard SONAR-netCDF4 variables are used. It assumes that tilt_x/y variables have already been transformed into pitch & roll. I'll create a new issue for this.
  • I learned recently (from the EK80 reference manual) that EK80 TransducerAlphaX/Y/Z variables are in degrees. We're currently interpreting them as a unit vector when mapping them to beam_direction_x/y/z. A conversion will need to be implemented in set_groups_ek80 (and ek60?). I'll create a new issue for this. For the sample file (below), it's not an issue b/c all TransducerAlphaX/Y/Z values are zero. I couldn't find an EK80 file with TransducerAlphaX/Y/Z values that are not all zero.
  • We don't have a sample EK60/80 raw file that has all relevant variables populated with what appear to be valid values. The one file I'm using, from 2021 Hake survey ("ek80/ncei-wcsd/SH2106/EK80/Reduced_Hake-D20210701-T131621.raw"), only has some of the variables with apparently valid, non zero data: pitch, roll, and vertical_offset. vertical_offset (based on heave) is also suspicious b/c values are close to zero. water_level (0) + vertical_offset is supposed to be the positive distance from the ship origin.

@emiliom emiliom added the enhancement This makes echopype better label Oct 28, 2023
@emiliom emiliom added this to the 0.8.2 milestone Oct 28, 2023
@emiliom emiliom self-assigned this Oct 28, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2023

Codecov Report

Merging #1205 (43e44cd) into dev (9dfe5ba) will increase coverage by 15.77%.
Report is 3 commits behind head on dev.
The diff coverage is 97.61%.

@@             Coverage Diff             @@
##              dev    #1205       +/-   ##
===========================================
+ Coverage   77.08%   92.85%   +15.77%     
===========================================
  Files          67        3       -64     
  Lines        5921      196     -5725     
===========================================
- Hits         4564      182     -4382     
+ Misses       1357       14     -1343     
Flag Coverage Δ
unittests 92.85% <97.61%> (+15.77%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
echopype/consolidate/api.py 94.95% <97.61%> (+1.13%) ⬆️

... and 64 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@emiliom
Copy link
Collaborator Author

emiliom commented Oct 30, 2023

Notes to self for creating mock EchoData data for testing the new functionality. Here's the minimal code needed to create a bare EchoData object that has just the elements needed by consolidate.add_depth: a sonar_model attribute and the "Platform" and "Sonar/Beam_group1" groups.

from datatree import DataTree
import xarray as xr
from echopype.echodata.echodata import EchoData

ed = EchoData(sonar_model="EK80")

ds1 = xr.Dataset(dict(bar=("x", [1, 2])))
ds2 = xr.Dataset(dict(bar=("x", [10, 20])))
tree = DataTree.from_dict({'Platform': ds1, 'Sonar/Beam_group1': ds2}, name='root')
ed._set_tree(tree)

@emiliom emiliom closed this Oct 30, 2023
@emiliom emiliom reopened this Oct 30, 2023
@emiliom
Copy link
Collaborator Author

emiliom commented Nov 7, 2023

Added a test function to generate mock EchoData object with only the variables (and one attribute) required by consolidate.add_depth. This function partially populates only the Platform, Sonar and Sonar/Beam_group1 groups, plus root by default. Also added a test (test_add_depth_from_echodata_mock) that uses this mock data, together with a mock Sv Dataset.

@leewujung leewujung modified the milestones: 0.8.2, 0.8.3 Nov 20, 2023
@leewujung leewujung removed this from the 0.8.3 milestone Mar 14, 2024
@leewujung leewujung assigned ctuguinay and unassigned emiliom Mar 28, 2024
@leewujung
Copy link
Member

@ctuguinay : This is also related to #1152. Let's talk through this.

@leewujung
Copy link
Member

We'll close this now because it'll be superseded by #1319 .

@leewujung leewujung closed this May 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This makes echopype better
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants