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

Notebook demonstrating glider AZFP data with external glider data #31

Closed
wants to merge 5 commits into from

Conversation

emiliom
Copy link
Collaborator

@emiliom emiliom commented May 5, 2023

Draft notebook demonstrating glider AZFP data processed with external Platform and Environment data. Data provided by Rutgers.

Unlike other echopype-examples notebooks so far, this one relies on small data files uploaded to the repo in a new data directory. Files include AZFP .01A and .XML files, and a Slocum glider netcdf file encompassing the time period spanned by the AZFP data.

Platform data is inserted into the echodata object using the update_platform method. Calibration is performed with temperature, salinity and pressure data averaged from the data in the Slocum glider file (come to think of it, I should probably average only over the time range spanned by the AZFP data). Location and depth are added to Sv using ep.consolidate.add_location and ep.consolidate.add_depth.

@leewujung at this time please review only the code, the sequence of processing steps and the plots and print outs. I will fill in the introductory text later, will edit the notebook title, and revisit all section headers and annotation text. Also, I'm using echopype.visualize.create_echogram because I wanted to try to showcase that echogram plotting function, but I don't like that I wasn't able to find a way to display the frequency (frequency_nominal) rather than the channel ID.

…and environment data; includes small data files in new data directory
@emiliom emiliom requested a review from leewujung May 5, 2023 01:36
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@emiliom emiliom marked this pull request as ready for review May 22, 2023 08:43
@emiliom
Copy link
Collaborator Author

emiliom commented May 22, 2023

I've finalized the notebook and references to it in the Jupyter Book main page and table of content.

The only remaining question I have is the use of echograms using ep.visualize. @leewujung should we keep it, or replace it with more "manual" plotting steps as done in the other notebooks? See the comments I've left in that section, in the notebook.

@lsetiawan lsetiawan closed this May 23, 2023
@lsetiawan lsetiawan reopened this May 23, 2023
Copy link
Member

@leewujung leewujung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emiliom : thanks for putting together this notebook. Below are some comments that are smaller:

  • recommend changing the filename to glider_azfp_externaldata.ipynb
  • most of the Platform group data variables that do not exist in the .01A file are stored as a single NaN, but latitude and longitude just do not exist at all. Could you standardize this as part of review with CDL #1043?
  • would be good to mention that in updating the latitude and longitude, these values are interpolated using the external dataset
    • one thing we should think about here is whether to mention how this interpolation is done -- in a sense we are not doing it correctly since we just do linear interpolation without taking into account the geodesic

The main thing that prevent this from being merged is that the consolidate.add_depth function is not ready to do what needs to be done for gliders or similar platforms that moves in the Z (depth) dimension. If you look at the add_depth operation, it does not currently take in the true depth of the platform and then add/subtract (depending on transducer orientation) the echo_range correctly to get depth.

This is a small addition, but I won't be able to get to it anytime soon...

I will submit a PR to your branch for some wording changes.

@emiliom
Copy link
Collaborator Author

emiliom commented May 23, 2023

  • recommend changing the filename to glider_azfp_externaldata.ipynb

I like that. Plus, I see that using underscores is more consistent with the names of the other notebooks. I'll change it.

  • most of the Platform group data variables that do not exist in the .01A file are stored as a single NaN, but latitude and longitude just do not exist at all. Could you standardize this as part of review with CDL #1043?

Yeah, I noticed that early only, and I intend to do something about it. FYI I recently asked a broader question about this in the SONAR-netCDF4 repo.

  • would be good to mention that in updating the latitude and longitude, these values are interpolated using the external dataset
    • one thing we should think about here is whether to mention how this interpolation is done -- in a sense we are not doing it correctly since we just do linear interpolation without taking into account the geodesic

Ok, will do.

The main thing that prevent this from being merged is that the consolidate.add_depth function is not ready to do what needs to be done for gliders or similar platforms that moves in the Z (depth) dimension. If you look at the add_depth operation, it does not currently take in the true depth of the platform and then add/subtract (depending on transducer orientation) the echo_range correctly to get depth.

This is a small addition, but I won't be able to get to it anytime soon...

Right. Maybe we can chat about this briefly tomorrow?

I will submit a PR to your branch for some wording changes.

Thanks!

@emiliom
Copy link
Collaborator Author

emiliom commented May 26, 2023

Resolution of the depth issue in this notebook is contingent on progress on OSOceanAcoustics/echopype#1051

@ctuguinay
Copy link
Collaborator

Closing this since the glider notebook will be put in with #52

@ctuguinay ctuguinay closed this Aug 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants