Add Region Browser with interactive editing functionality (#220, #219)

[Safwannn89]

Implemented interactive region browser with Browse/Edit mode toggle and polygon editing capabilities.

Closes #220
Related to #219

Features:

Browse Mode

• View-only mode with clean interface
• Dropdown region selector + Previous/Next navigation

Edit Mode

• Interactive polygon editing with PolyDraw tool
• Click to add vertices, drag to move, double-click to delete
• Save changes to DataFrame
• Actions section with Save/Reset/Export buttons

Data Management

• Load CSV: Import saved regions with validation
• Save Changes: Update DataFrame in memory
• Export CSV: Save to file with timestamp
• Reset: Restore original from backup

Validation

• Time range check (regions must match echogram times)
• Format check (required columns: region_id, time, depth)
• Data type check (datetime64[ns], float64)
• Clear error messages for invalid data

Technical Details

Format: Migrated to Echoregions standard (region_id as int, time/depth as numpy arrays)

Files:

• docs/source/version_0.1.0/develop_region_browser.ipynb
  (Organized into 4 main sections: Setup & S3 Connection, Sample Data, Parser, and Interactive Dashboard)

Testing

• Browse/Edit mode switching
• Polygon editing (add/move/delete vertices)
• Save/Load functionality
• Validation with invalid data (correctly rejects)
• Export to CSV
• Real S3 echogram data

Test Results:

• Valid CSV: "Loaded 4 regions from CSV"
• Invalid CSV (wrong dates): "Validation failed: Region times outside echogram range"

Screenshots

Browse Mode:

Browse mode with dropdown navigation

Edit Mode:

Edit mode showing Actions section

Validation:

Validation error message

Ready for:

• Testing with real EVR files
• Additional features as needed

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[codecov-commenter]

Codecov Report

❌ Patch coverage is 5.20000% with 237 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.13%. Comparing base (a6ecd8b) to head (2dccea1).
⚠️ Report is 75 commits behind head on main.

Files with missing lines	Patch %	Lines
echoshader/region_browser.py	4.04%	237 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #226       +/-   ##
===========================================
- Coverage   72.88%   52.13%   -20.75%     
===========================================
  Files          11       12        +1     
  Lines         649      938      +289     
===========================================
+ Hits          473      489       +16     
- Misses        176      449      +273     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[LOCEANlloydizard]

Hi @Safwannn89, I've seen your PR, I should get back to you Wednesday! thx!

[Safwannn89]

Hi @LOCEANlloydizard,

Thanks for getting back to me!

I wanted to share something I noticed after making this PR. I realized that relying only on PolyDraw wasn't ideal because it struggles to delete specific vertices (it's best used for adding and dragging).

To give the user complete editing control, I think I will integrate PolyEdit alongside it.

Here is how they would work together:

PolyDraw: Add and move vertices.

PolyEdit: Delete vertices (double-click, then press ESC).

Since they both share the same polygon data, the user experience will be much smoother now.

Please give your feedback on this!

Thanks

[LOCEANlloydizard]

Hi @Safwannn89,

Alright for the poly_draw and poly_edit, if that's smoother, then good! I guess we can still adapt/tune it later if needed, once we deploy and see how it goes.

One main change needed here: we should move the logic into modular functions so it can be reused elsewhere (and long-term integrated into a main app that call the different components/panels).

For instance, the code should live under echoshader/echoshader/, similar to those scripts:

Then in the notebook (which would keep the same content, I believe), you’d just call your function like:

browser = echoshader.region_browser(
    ds=MVBS_ds,
    regions=regions_df,
    other options...
)
browser

Talking to @leewujung, she also mentioned we could look into xarray accessors: Extending xarray using accessors. See echoshader/core.py for how it’s implemented in echoshader. So the region browser could also fit this patern, e.g.:

browser = MVBS_ds.eshader.region_browser(regions_df, ...)
browser

The main code would still live in something like echoshader/region_browser.py, wth just a thin accessor method added in core.py. (does that makes sense?)

One open question: do we want something like

echoshader/regions/browser.py
echoshader/regions/editor.py

or should the editor come directly with the browser? (sorry if we already discussed this point!) Happy to hear your thoughts/ideas on this. Cheers!

[LOCEANlloydizard]

@Safwannn89 following up on the review: just to clarify that we really don't necessarily have to go down the accessor route (just something to keep in mind). After a bit more discussion with @leewujung, not using the accessor pattern might actually be simpler for users, especially for more advanced functions like the region browser.. (But we would still need the .py script!)
Happy to hear your thoughts! Cheers!

[Safwannn89]

Hi @LOCEANlloydizard,

Thanks for clarifying! I'll go with the simple function approach (no accessor pattern needed).

My Plan:

I'll create a region_browser() function in echoshader/region_browser.py that does everything browsing and editing in one tool with a mode toggle.

Usage would be simple:

import echoshader

browser = echoshader.region_browser(ds=MVBS_ds, regions_df=regions)
browser.show()

The notebook becomes much cleaner just load data and call the function!

About the Browser vs Editor Question:

I think keeping them combined (which is option A) makes more sense for a few reasons:

The way I built it, Browse and Edit are just two modes in the same tool users toggle between them with a button. They share all the same components: navigation, echogram display, region selection, etc.

Let me know what you think about this approach, then i will move forward.

Thanks!

[LOCEANlloydizard]

Hi @Safwannn89, yes I think its good like this! Thank you!

[Safwannn89]

Hi @LOCEANlloydizard and @leewujung,

I've refactored the Region Browser and moved all logic into echoshader/region_browser.py so it can be imported and reused:

import echoshader
browser = echoshader.region_browser(ds, regions_df)
browser.show()

• Used simple function approach (not accessor) as discussed
• Kept browser and editor in one file since they share all components

The notebook is now much simpler, just 3 cells to launch the browser!

Let me know if you'd like any changes to the structure and also, I'm ready to test with real EVR data when you have it available!

Thank you!

[LOCEANlloydizard]

• add more markdown text?
• remove icon?

Reply via ReviewNB

[LOCEANlloydizard]

Line #4.    # Create sample hake school regions in Echoregions standard format

• Put this as Markdown?
• remove icons

Reply via ReviewNB

[LOCEANlloydizard]

Line #9.    print("✅ Browser ready!")

maybe this is not useful? if the "Launching server at.." anyways comes up?

Reply via ReviewNB

[LOCEANlloydizard]

Hi @Safwannn89, thanks for the update! Nice to see the shorter notebook also!

I have a small comment below. Also, after addressing the first comments, I think you can request another round of review so I can take another look :)

Notebook points

• I think the notebook needs a bit more context to explain the different steps. For instance, instead of using an inline comment like "Create sample hake school regions in Echoregions standard format", this could be a short markdown section instead, maybe with a reference/link to the echoregions repo/RTD? More generally, a bit more narrative text would help!

• Maybe the notebook could simply be named region_browser.ipynb?

• I would remove the icons from the notebook, and probably most of them from the app as well, unless they are especially appropriate. For example the save icon could maybe stay, but for instance I would remove the fish icon?

• We would still need the notebook outputs committed so they show up in the docs/ReadTheDocs. Also, instead of print("✅ Data loaded successfully"), maybe we could instead display the ds_ooi content?

• Instead of creating the regions directly in the notebook (second block), could we perhaps launch the program with an example CSV file attached to the PR (containing the 4 region), and then add text to explain to users how to import it in the app'? That might feel a bit closer to a real user case? Just an idea, happy t hear your thoughts!

Implementation

• In the code, when errors are raised, maybe we could make the error messages a bit more explicit about what happened?

• Also, regarding the buttons, unless I misunderstood, the current logic seems to be:

  • Reset restores the original loaded region (that you copy), not the last saved state

  • Save does not write to disk; it only updates sample_df in memory

  • Export CSV is the step that actually writes a new file

So if a user wants to save their work, they still need to export the result to CSV. I wonder if we should make that more explicit, because “Save” sounds like it saves the work to disk. Also, if a user edits several regions and then clicks "Reset" by mistake before exporting, they would lose those edits, right? So I was wondering whether the Reset button should be this accessible, or even whether this functionality is really needed, since it seems users can already double-click a polygon to remove it?

• Finally, I opened a branch with your PR and tried it locally. Maybe I’m using it wrong, but I got a few issues while playing around with the browser/editor.

  • For the browser, I found it a bit slow to navigate between regions. Did you notice the same on your side? Just wondering whether there are ways to make that faster?

  • Also, I was unable to create a new region. If I switch to Edit mode and try to draw on the echogram, I can still navigate around the echogram rather than draw a polygon. Maybe I mised something obvious on my side, sorry if so!

Cheers!

[Safwannn89]

Hii @LOCEANlloydizard

Thanks for taking the time to provide such a thorough review. I am currently working on your feedback and implementing the necessary changes. I will reach out to you as soon as I'm done.

Thanks!

[Safwannn89]

Hi @LOCEANlloydizard,

I've pushed updates addressing all your points, along with some PEP8/flake8 style fixes.

Notebook & UI Updates:

Renamed to region_browser.ipynb and removed emojis for a cleaner look.

Added narrative Markdown context, RTD links, and now display the native ds_ooi output.

Created sample_hake_regions.csv to demonstrate a realistic import workflow directly in the notebook. Outputs are committed for ReadTheDocs.

Tool and Bug Fixes:

Drawing Tools: Fixed! The tools now properly attach to the overlay and reliably appear in the toolbar when switching to Edit mode. Added explicit UI instructions for using them.

Reset Button: I re-engineered this based on your feedback. Instead of a global reset, it now safely restores only the currently viewed polygon to its original loaded state. This acts as a safe "Undo" if a user accidentally ruins a shape, without forcing them to lose all other edits by reloading the CSV. Happy to remove it if you still prefer!

Save vs Export: Renamed "Save" to "Apply Edit" and added a highlighted UI warning so users know they must click "Export to CSV" to write changes to disk.

Error Handling: Added explicit, color coded UI alerts for validation and parsing errors.

Regarding the Speed/Lag Issue:
I investigated the navigation lag. Because the UI currently uses @pn.depends, Panel forces a full re-render and re-serialization of the massive echogram over the network on every mode switch.

To fix this properly, we need to refactor this component to use hv.DynamicMap. This will freeze the echogram in the browser and only stream the tiny polygon coordinate changes via WebSockets. Since this is a major architectural rewrite, I didn't want to risk breaking the current working CSV logic in this PR.

Would you prefer I open a separate Issue to track a DynamicMap refactor for a follow-up PR, or include it here? Happy to discuss the best approach!

Ready for another look!

Cheers,

Safwan

[Safwannn89]

Hi @LOCEANlloydizard ,

Hope you're doing well! Just a gentle nudge on PR #226, I've addressed all the feedback from your previous review, and the PR is ready for another look whenever you have a chance.

Thanks!
Safwan