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

Refactor high-level API by grouping functionality under sub-objects #140

Closed
confluence opened this issue Sep 26, 2023 · 2 comments · Fixed by #123
Closed

Refactor high-level API by grouping functionality under sub-objects #140

confluence opened this issue Sep 26, 2023 · 2 comments · Fixed by #123
Assignees

Comments

@confluence
Copy link
Collaborator

confluence commented Sep 26, 2023

I've been thinking about how to organise both the code and the user API to break up the huge image and session classes and group related functionality together, and I think that it would be good to split some items into sub-properties on these objects. This would make the code less unwieldy and also simplify some of the API naming. So far some groups I've identified are: the overlay (session), and the raster, contours, vector overlay, and the about-to-be added regions (image). At a later stage we will probably add other functionality that follows the same pattern. Some functionality (e.g. inspecting image geometry and other properties) I consider to be "basic" functionality which can remain accessible directly from the main object. I am considering adding an images group to the session object, for consistency with the regions group I'm planning to add to the image object (the image-opening functions would be grouped under this object).

This would change the user-facing API, so I think we should do it sooner other than later (and it may also affect the e2e tests). My suggestion would make the API look something like this:

session.overlay.set_coordinate_system(...)
session.images.open(...) # maybe?
img.raster.set_colormap(...)
img.contours.set_colormap(...)
img.vectors.set_colormap(...) # simpler than vector_overlay?
# To be added in upcoming region implementation
img.regions.add_rectangular(...)

Does that make sense?

I was considering transparently aliasing these functions to functions on the main session and image objects (for backwards compatibility), but 1) that would be a lot of boilerplate, and 2) it would complicate the naming. So I think we should commit to the backwards-incompatible change now, before the wrapper has been more widely released.

Originally posted by @confluence in #123 (comment)

@confluence confluence self-assigned this Sep 26, 2023
@confluence
Copy link
Collaborator Author

This was discussed in a comment and on Slack, but didn't have an actual issue.

To do:

  • Factor code for simplifying sub-object call_action, etc., out of Image so that it can be reused
  • Existing functions should be refactored: in Session: overlay (see refactor AST-overlay related functions  #119); in Image: raster, contours, ???
  • Update open feature PRs

@confluence
Copy link
Collaborator Author

This will be resolved by #123.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Icebox
1 participant