Update Notebooks

[ctuguinay]

• Primary Changes:
  • Placed all functionality for both classes in 1 notebook.
  • Notebook order is as follows: Parse EVL/EVR -> Look at DataFrame / manipulate it -> Load and plot Sv -> Show masking functionality -> Show saving functionality.
  • All data now is being grabbed from the feature branch. I'll change it to the main branch right before I merge this branch.
  • I kept in the old notebooks just as a reference. I will remove them before merging.
• Secondary Changes:
  • Made use of select_region earlier in r2d.mask to simplify both region_id selection and mask_labels creation (when passed in mask_labels is None)
  • Modified EVR trawl region to make it smaller
  • Fix region plot bug where dataframe was being passed into close_region even though close_region doesn't take in dataframes
  • Added kwaargs to to_csv
    • Index value should be something the user wishes to pass in

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.56%. Comparing base (8f58a52) to head (c8e7640).
Report is 1 commits behind head on main.

❗ Current head c8e7640 differs from pull request most recent head ca85153. Consider uploading reports for the commit ca85153 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #184      +/-   ##
==========================================
- Coverage   90.58%   89.56%   -1.02%     
==========================================
  Files          13       13              
  Lines         616      556      -60     
==========================================
- Hits          558      498      -60     
  Misses         58       58              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[valentina-s]

I did a full reread of the notebooks (so not just the recent changes). Here are my comments in sequence (let me know if it is not clear which lines I refer to):

Getting Started

• tool -> python package

• "Echoregions is a tool that interfaces annotations of water column sonar data with Machine Learning (ML) models." -> "Echoregions is a tool that interfaces with annotations of water column sonar data for training machine learning models or doing other downstream processing such as biomass estimation".

• manual annotations -> turns out some annotations from Echoview are automatic annotations. Maybe we can say directly "Echoview" annotations (since this is the focus now)

• Mention Echoview earlier

Lines Functionalities:

• Installation: add instruction for installation from pypi

• “masks them over corresponding sonar data“ -> create a bottom mask for the corresponding sonar data.

• website link -> maybe just have a short handle

• 0 = none 1 = unverified 2 = bad 3 = good -> could write them on a separate line or use semi colon or something so that it is more readable

• “For usage … let’s put set” set

• introduce echopype link when first mentioning it.

• However, for machine learning training purposes, or for biomass estimation, it is necessary to mask out the pixels below the bottom line.

• “The mask function first interpolates on the bottom points found in the DataFrame and generates new points bottom points that match the ping time coordinates of the sonar data passed in, and from these new interpolated bottom points, generates an xarray bottom mask.” -> “interpolates the bottom points found in the DataFrame to generate new points bottom points”

• Being more explicit that it operates on the ds_Sv[“Sv”] vs ds_Sv: what happens if we pass ds_Sv

• bottom_contours: I usually think of contour as something closed (although I know it is is not necessary). I wonder if bottom boundary is better or maybe there is another term?

• "Let’s take a look at the mask bottom_mask_da first:" was something expected after the column

• Missing quote: ds_Sv["Sv]

• explain that 1 is below the bottom line

• what happens if you add Sv without limiting to one channel

• mask exists -> mask is 1

• bottom_contours (often contour is through to be closed)

• ping time dimension of the passed in dataset

• spline could have a zoomed-in plot example to see what it is doing

• CSV -> .csv format (be a bit more formal in the language)

• sonar dataset language -> I am not sure if at some point we should change the sonar dataset language to "echosounder": it a bit depends on the user community: i.e. fisheries acoustics researcher, or a student training an ML model. We can decide this in the future.

Regions 2D Functionalities:

• trawl_region? I think you want =“Trawl”? The dataframe shows as empty?

• Same point about adding a link when introducing echopype and having shorter link handles for the other references.

• How many regions are in this dataset: seems you are looping through them but there is just one?

• "We can see that there is clear overlap between the two pieces of data here." ->

• "We can see there is a faint dispersed region, which represents Hake distribution." It is a bit hard to see for a new-comer! The ek500 colormap can help.

• point data -> region polygon? to a pixel mask indicating which pixels are inside the polygon and which are outside.

• could mention that it is using the regionmask package under the hood

• explain the "3d" in mask3d a bit more since this crucial to understanding the format of the mask: maybe point to what the dimensions are and link to some references from regionmask. An example with two regions may be more helpful for the explanation. You can link to the built-in documentation for more details.

• mask exists -> mask is 1

• So now that we have our mask and our new interpolated region points, how do we save them? -> no interpolated points here?

• Maybe switch to the ek500 colormap if it does not add extra echopype requirements

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[ctuguinay]

@valentina-s Thanks for the review! I added Echoregions to ReviewNB incorporated most of your changes with just a few comments of my own:

Getting Started

manual annotations -> turns out some annotations from Echoview are automatic annotations. Maybe we can say directly "Echoview" annotations (since this is the focus now)

Mention Echoview earlier

In the previous revision of the description, Wu-Jung made changes that were less Echoview-focused so that this package was not locked into Echoview. I think I agree with these changes too since the annotations don't necessarily need to come from EVL EVR, for instance we can create annotations with our model (in this case it would not be manual though 😆).

Regions2d and Lines

bottom_contours: I usually think of contour as something closed (although I know it is is not necessary). I wonder if bottom boundary is better or maybe there is another term?

bottom_contours (often contour is through to be closed)

Perhaps in this PR, we can sort out this naming issue. I think bottom boundary does not reflect what the data we actually get is: points. Perhaps bottom boundary points? Or bottom boundary vertices? I am also thinking of changing the region contour name with something like region boundary points/vertices.

[valentina-s]

Echoregions' Github repository

EVL data -> evr data

EVL file -> evr file

Reply via ReviewNB

[valentina-s]

You say hake region but select "Trawl"

Reply via ReviewNB

[valentina-s]

thses -> these

Reply via ReviewNB

[valentina-s]

I guess Sonar does not need to be capitalized.

Reply via ReviewNB

[valentina-s]

The title is a bit confusing:

Platting Sonar and Region works, but masking should be masking a sonar with a region? Maybe you can split the sections: Plotting Sonar and Region, and next section: Masking Sonar by Region?

Reply via ReviewNB

[leewujung]

For some reason this caught my eye while I tried to delete the auto-generated emails from GitHubs. Please change "sonar" to echogram. Sonar is an instrument, echogram is the image formed by received sonar echoes.

[valentina-s]

can you ad to the first sentence: masks of the sonar data, which indicate which points are within the region.

for the dimension you can use maybe quotes ping_time and depth

Reply via ReviewNB

[valentina-s]

Look -> lower case

Reply via ReviewNB

[valentina-s]

the points that constitute the region mask -> the points that constitute the region

Reply via ReviewNB

[valentina-s]

A plot of the mask for region id 18

Reply via ReviewNB

[valentina-s]

could add quotes for the file formats .csv

Reply via ReviewNB

[valentina-s]

create -> creates

Reply via ReviewNB

[valentina-s]

.evl data from the Echoregions' Github repository

Reply via ReviewNB

[valentina-s]

Can shorten this link too.

Reply via ReviewNB

[valentina-s]

Given that the ek500 colormap made the bottom very red, and the red points became invisible: it is better to switch to anotehr color for the points.

Reply via ReviewNB

[valentina-s]

xarray -> xarray or Xarray, for pandas you have it capitalized. You can decide on a way to represent the libraries and be consistent with it.

Reply via ReviewNB

[valentina-s]

It is interesting that the smoothed one also has a lot of points but that is more about our data than the package itself.

Reply via ReviewNB

[valentina-s]

You can switch to .csv and .csv in the title

Reply via ReviewNB

[valentina-s]

Regarding bottom_contours: we could just call them bottom points since echoview also talks about bottom points at some places around its documentation.

Reply via ReviewNB

[valentina-s]

@valentina-s Thanks for the review! I added Echoregions to ReviewNB incorporated most of your changes with just a few comments of my own:

Getting Started

manual annotations -> turns out some annotations from Echoview are automatic annotations. Maybe we can say directly "Echoview" annotations (since this is the focus now)
Mention Echoview earlier

In the previous revision of the description, Wu-Jung made changes that were less Echoview-focused so that this package was not locked into Echoview. I think I agree with these changes too since the annotations don't necessarily need to come from EVL EVR, for instance we can create annotations with our model (in this case it would not be manual though 😆).

Regions2d and Lines

bottom_contours: I usually think of contour as something closed (although I know it is is not necessary). I wonder if bottom boundary is better or maybe there is another term?

bottom_contours (often contour is through to be closed)

Perhaps in this PR, we can sort out this naming issue. I think bottom boundary does not reflect what the data we actually get is: points. Perhaps bottom boundary points? Or bottom boundary vertices? I am also thinking of changing the region contour name with something like region boundary points/vertices.

Thanks for adding ReviewNB @ctuguinay, it was very nice to use it! I went through the notebooks again and marked some small things which you can see in the viewer.

• regarding the annotations: maybe we just call them annotations (no need to say manual, often that is assumed anyway). And the first time you introduce Echoview software, provide a link to it

• for the bottom: how about just bottom points: Echoview uses this terminology here and there discussing how a point is part of the bottom?

[ctuguinay]

@valentina-s Thanks for the 2nd round of review! I incorporated all your suggestions. An additional thing I added/changed (based on Wu-Jung's suggestion) was the use of the words sonar, backscatter/Sv data, and echogram. I was using them interchangeably prior, but I tried to make a more clear distinction between them. Let me know what you think of these changes.

[valentina-s]

@ctuguinay Could you just change Echoregion's to Echoregions' since the "s" is part of the package name (in both notebooks). Otherwise it all looks good and the notebooks run fine.