-
-
Notifications
You must be signed in to change notification settings - Fork 54
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
update quickstart with the tile locator, add locator for all scorer and fix negative levels #212
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please do the same adjustment suggested in the readme also in the quickstart.rst. Furthermore i'd like to mention the possibility to use the locate tiles even in the ScoreTiler, justo to inform that this method is available for each scorer in histolab
910fdbe
to
1fdf766
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see why to generate the tiles and the scores twice (if we save the report) by getting the scores and the scaled scores separately. I guess we are duplicating the execution time because we are calling self._tiles_generator(slide)
twice.
We could @lru_cache
the method _tiles_generator
so with different slide parameter the result is recalculated.... but I'm not sure, @ernestoarbitrio what do you think?
The problem here is that for the locate tiles (scorer) we can't have a method that returns 2 values, otherwise we cannot generalize that method. The idea of just parametrizing with the scored(bool) was just a POC for having some workable and usable thing. Myabe this can be refactored in a separate PR ... i can take care of it ... lmk what you think. If you agree just merge this and re-open a new issue and assign it to me :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ernestoarbitrio alright, approving this and opening a new issue
612d44c
to
e481035
Compare
Resolve issue 207