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

(Tentative) Deprecate image loader #335

Open
saeliddp opened this issue May 21, 2024 · 2 comments
Open

(Tentative) Deprecate image loader #335

saeliddp opened this issue May 21, 2024 · 2 comments
Labels

Comments

@saeliddp
Copy link
Collaborator

Issue

The CurationImageLoader currently violates the design principles behind the original curation refactor I did. This is because our model needs a ref to the image loader, and the image loader runs async tasks. One of the main ideas in the original refactor was that async code would be handled by a 'service' object, which neither the view nor the model would need direct reference to (rather, they would interact via signals).

I'm not sure how I overlooked this in the original refactor, but here we are--the reason I noticed this issue was that I was having some trouble writing tests.

Proposed Solution

The solution I've thought of mainly revolves around modifying the CurationModel to have the following attributes:

  1. curation record (already exists): a list of curation data for each set of images
  2. curr_img_data (new): a dict of {'raw': ImageData, 'seg1': ImageData, 'seg2': ImageData}... seg2 is optional
  3. next_img_data (new): a dict of {'raw': ImageData, 'seg1': ImageData, 'seg2': ImageData}... seg2 is optional
  4. curation_index (new): the index of the curr set of images

It would work something like this:

  • CurationModel would have a cursor_moved signal
  • CurationService would listen for that signal, when it receives it, it will inspect the state of the model, and start threads to extract image data for the necessary images
  • When one of those threads finishes, CurationService will write the data to CurationModel
  • Upon getting one of these writes, CurationModel can decide whether or not to emit a loading_finished signal, which can then be reacted to by the main view

Pros and Cons

PROS:

  1. no need to worry about mocking / faking the image loader (there is currently a lot of code related to the loader and its fake)
  2. can test the view and model without instantiating any objects that might run async tasks, which gives full control over when certain signals fire (which is good for testing)
  3. would be good for standardization of the design strategies in this project

CONS:

  1. This would probably be a sizable PR (though not nearly as big as the initial refactor). It would affect the model and service, but not the view
  2. CurationModel is already kind of large, this would likely increase its size
@saeliddp
Copy link
Collaborator Author

@hughes036 @yrkim98 lmk what you guys think when you get a chance, thanks!

@hughes036
Copy link
Collaborator

I like the idea! We should first check if there are any higher priorities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants