Closed ctuguinay closed 7 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 89.56%. Comparing base (
8f58a52
) to head (c8e7640
). Report is 1 commits behind head on main.:exclamation: Current head c8e7640 differs from pull request most recent head ca85153. Consider uploading reports for the commit ca85153 to get more accurate results
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Check out this pull request on
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
@valentina-s Thanks for the review! I added Echoregions to ReviewNB incorporated most of your changes with just a few comments of my own:
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 😆).
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 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?
@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.
@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.
select_region
earlier inr2d.mask
to simplify bothregion_id
selection andmask_labels
creation (when passed inmask_labels
is None)plot
bug where dataframe was being passed intoclose_region
even thoughclose_region
doesn't take in dataframesto_csv