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

🎨 New EngineABC Design for Patches #635

Merged
merged 153 commits into from
Oct 31, 2023

Conversation

shaneahmed
Copy link
Member

@shaneahmed shaneahmed commented Jul 6, 2023

  • New EngineABC design
  • Update PatchPredictor based on the new design
  • Move model_to from utils.misc to model_abc
  • Define load_torch_model in model_abc

- Refactor base code from IOSegmentorConfig to IOConfigABC
- No need for a separate function
- Enable git workflow on this PR
- Add missing variable
- Move `to_baseline` code to ABC
- Move `to_baseline` code to ABC
- Fix incorrect calculations for output_resolutions
- Update `PatchPredictor` to use `ModelIOConfigABC`

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Define `ModelIOConfigABC` as a dataclass

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Remove kwargs.
- Define dataclass attributes.

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Add `IOInstanceSegmentorConfig`

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Update yaml for  `IOInstanceSegmentorConfig`

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Update yaml for  `IOInstanceSegmentorConfig`

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Update `to_baseline` for  `IOInstanceSegmentorConfig`

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Update  IOInstanceSegmentorConfig in test_nucleus_instance_segmentor.py

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Update cli for IOInstanceSegmentorConfig

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Remove unnecessary test after kwargs removal

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Move ioconfigs to io_config.py

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Fix circular import

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Refactor to reduce code in inheriting class

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Fix deepsource errors

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Fix ioconfig import to avoid circular imports

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Fix missing input arguments

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Improve error catching

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Default configuration is not correct

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Fix error check

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Fix typo

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
- Fix unnecessary update

Signed-off-by: Shan E Ahmed Raza <13048456+shaneahmed@users.noreply.github.com>
@AbishekRajVG
Copy link
Contributor

Review comments addressed and pushed. I've only made changes to engine_abc and test_engine_abc. The misc.py changes are moved to a separate branch dev-persist-patchpredictor-output and are tracked in PR #724

Copy link
Collaborator

@mostafajahanifar mostafajahanifar left a comment

Choose a reason for hiding this comment

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

Thanks, Shan and collaborators for working on this. Looks like to is in good shape. I only made a couple of suggestions to consider.

tiatoolbox/models/models_abc.py Show resolved Hide resolved
tiatoolbox/models/models_abc.py Show resolved Hide resolved
tiatoolbox/models/engine/engine_abc.py Show resolved Hide resolved
num_postproc_workers (int):
Number of workers to postprocess the results of the model. default = 0
verbose (bool):
Whether to output logging information.
Copy link
Collaborator

@mostafajahanifar mostafajahanifar Oct 20, 2023

Choose a reason for hiding this comment

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

Actually, it would be nice to give the user option set the path to save engine logs for a specific run.

Shan: let's do it in a separate PR.

Abishek: EngineABC's verbose option displays the progress bar. Should we really have an option to save this to a file?

________________________________________________________________ test_engine_run_with_verbose _________________________________________________________________
-------------------------------------------------------------------- Captured stderr call ---------------------------------------------------------------------
100%|#############################################| 2/2 [00:00<00:00,  5.37it/s]

         progress_bar = None

        if self.verbose:
            progress_bar = tqdm.tqdm(
                total=int(len(data_loader)),
                leave=True,
                ncols=80,
                ascii=True,
                position=0,
            )

tiatoolbox/models/engine/engine_abc.py Show resolved Hide resolved
tiatoolbox/models/engine/engine_abc.py Show resolved Hide resolved
Copy link
Member Author

@shaneahmed shaneahmed left a comment

Choose a reason for hiding this comment

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

Thanks @AbishekRajVG Looks good!

@shaneahmed shaneahmed merged commit a6cd508 into dev-define-engines-abc Oct 31, 2023
8 of 10 checks passed
@shaneahmed shaneahmed deleted the dev-redefine-patchpredictor branch October 31, 2023 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants