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

Simplify package design for label reading #88

Closed
leewujung opened this issue Jun 5, 2023 · 6 comments · Fixed by #95
Closed

Simplify package design for label reading #88

leewujung opened this issue Jun 5, 2023 · 6 comments · Fixed by #95
Assignees
Labels
setup infrastructure or packaging

Comments

@leewujung
Copy link
Member

The design of the label reading components contains the scaffolding that is too cumbersome for the couple simple geometries we are dealing with (polygons and lines). Let's simply the design to make reading labels into objects more straightforward.

@leewujung leewujung added the setup infrastructure or packaging label Jun 5, 2023
@leewujung leewujung added this to the Current status check milestone Jun 5, 2023
@ctuguinay
Copy link
Collaborator

@leewujung Which label reading components are you referring to? I am a bit lost as to what you mean by scaffolding in this context.

@leewujung
Copy link
Member Author

oh sorry, should make it more explicit. Label reading meaning to read EVR or EVL files. Scaffolding is the double layers of objects initiation needed to read these files.

@ctuguinay
Copy link
Collaborator

Oh, I see. Simplifying seems like a lot of changes need to happen in the convert modules. Let's make sure to talk about this in-depth during tomorrow's meeting.

@leewujung
Copy link
Member Author

leewujung commented Jun 7, 2023

Yep, it would be quite a bit of change, but I think it may be easiest to just build it from ground up by grabbing the necessary components, instead of trying to modify from the existing structure. Let's definitely talk about this tomorrow -- bring your thoughts!

@ctuguinay ctuguinay moved this to Todo in Echoregions Jun 7, 2023
@ctuguinay
Copy link
Collaborator

I'll add a few tasks needed to complete this task:

  • For parse_evr: Do enough refactoring and rearranging/removing of functions so that the inner function parse_file(evr_name) is only called once.
  • For parse_evl: Do enough refactoring and rearranging/removing of functions so that the inner function parse_file(evl_name) is only called once.

@ctuguinay ctuguinay moved this from Todo to Done in Echoregions Jun 13, 2023
@ctuguinay ctuguinay moved this from Done to In Progress in Echoregions Jun 13, 2023
@ctuguinay ctuguinay moved this from In Progress to Done in Echoregions Jun 14, 2023
@ctuguinay
Copy link
Collaborator

This will be done in the refactoring, since this heavily coincides with what refactoring is meant to do.

@ctuguinay ctuguinay reopened this Jun 23, 2023
@ctuguinay ctuguinay mentioned this issue Jun 22, 2023
@ctuguinay ctuguinay linked a pull request Jun 26, 2023 that will close this issue
@ctuguinay ctuguinay moved this from Done to Review Needed in Echoregions Jun 30, 2023
ctuguinay added a commit that referenced this issue Jul 2, 2023
### Refactoring

This PR closes the following issues: #91, #88, #83, #43.

**The main folders are now:**
- utils
- lines
- regions2d

**As opposed to before:**
- utils
- convert
- formats
- mask
- plot

**Refactoring notes:**
- `core.py` contains `read_evl` and `read_evr`.
- In both regions2d and lines, plotting is still kept as a class function: `r2d.plot() `and `lines.plot()`.
- In both regions2d and lines, masking is still kept as a class function: `r2d.mask() `and `lines.mask()`.
- In both regions2d and lines, parsing is kept outside of class: `parse_regions_file(input_file)` and `parse_lines_file(input_file)`.
- We now always initialize regions2d and lines with appropriate .evr and .evl functions at initialization ALWAYS. This is enforced with an error if not initialized properly (incorrect .evr or .evl file).

**Other Changes Include:**
- Added line masking based on Valentina's example notebook with options for what to do with NA values and what interpolation method to use.
@github-project-automation github-project-automation bot moved this from Review Needed to Done in Echoregions Jul 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
setup infrastructure or packaging
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants