Skip to content

Refactor write_evr to public API similar to read_evr and read_regions_csv - #291

Draft
ctuguinay wants to merge 6 commits into
echostack-org:mainfrom
ctuguinay:refactor_write_evr
Draft

Refactor write_evr to public API similar to read_evr and read_regions_csv#291
ctuguinay wants to merge 6 commits into
echostack-org:mainfrom
ctuguinay:refactor_write_evr

Conversation

@ctuguinay

@ctuguinay ctuguinay commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Addresses #243:

The write_evr/evr_writer.py was refactored/removed so that reading a mask now goes through a similar workflow as .csv and .evr files, with read_mask taking in the xarray mask and creating a Regions2D object and this function is calling an underlying parser handling mask checking and contour extraction in regions2d_parser.py. Once the mask data is in Regions2D form, we can choose to save to either .evr or to .csv. Included is a simple values check that the mask is being correctly encoded. The test constructs a 4x4 mask and then within that box there is a smaller 2x2 box that is 1s (everything else is 0s) and then the Regions2D time and depth arrays are easy to check against.

@ctuguinay

ctuguinay commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator Author

@LOCEANlloydizard @leewujung

Sorry, I know y'all are probably swamped with some things, but when one of you has time, could you take a look at this PR? I think this refactoring makes more sense than what I previously had which felt very disconnected from the other public API parts, but let me know if any of this doesn't make sense.

@leewujung leewujung left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hey @ctuguinay : I like this change! It is much cleaner than having a flying function to write to evr.

The only question I have is when the mask has a hole - in this case does open_cv return 2 contours, one the outer contour and one the inner contour? If so, then in this case they will become 2 separate region_id entries in an Regions2D object, is that right?

@ctuguinay

ctuguinay commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

@leewujung Ah good point, there's a hierarchy structure to the contour format that I didn't encounter in my simple example. Let me figure that out and change how the opencv output is parsed and I'll add an example with a hole.

I'll also add an example with two patches that are separated (not contained within each other).

@ctuguinay

ctuguinay commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

@leewujung Once you get back, could we briefly talk about the inner/outer region case? I found strange contours in one of my test cases (potentially a bug in the in opencv??). A little hard to explain with this thread.

@ctuguinay
ctuguinay marked this pull request as draft August 3, 2026 20:59
@leewujung

Copy link
Copy Markdown
Member

Sounds good! Let's chat tomorrow!

@ctuguinay

Copy link
Copy Markdown
Collaborator Author

Also just noting that merging this will close #103

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants