-
Notifications
You must be signed in to change notification settings - Fork 3
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
Fix sorting on scores #164
Conversation
fc57b5a
to
eb7993b
Compare
4cb801e
to
38fcae7
Compare
7fc0dc1
to
f510969
Compare
e224a44
to
e326e4e
Compare
3e5fa3c
to
3110baa
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.
Looks good, I just had a couple of optional comments
src/nrtk_explorer/app/transforms.py
Outdated
logger = logging.getLogger(__name__) | ||
logger.setLevel(logging.INFO) | ||
|
||
|
||
class LazyDict(Mapping): |
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.
Nice utility!
According to this Mapping
is for a read-only maps, and MutableMapping
is used for mutable maps. Also an implementation for __delitem__()
is missing.
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.
✅
payload = msg.get("payload", {}) | ||
logger.debug(f"Worker: Received {command} with ID {req_id}") | ||
|
||
if command == "SET_MODEL": |
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 think we should place these command strings in some constant/enum, since we refer to them in different parts of the application.
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.
Sounds good. Let me do that in another PR right now, as I've made a mess of the history and modifed this file in the following PRs.
Rathern than through member variable
If the NUM_IMAGES_DEFAULT was smaller than current num_images, then we would clamp num_images when switching datasets.
After updating the images visible in the image list, all other selected images are updated and their scores computed.
After images are scored, the table sort may have changed the images that are visible, so ImageList is asked to send visible image IDs again, which may trigger a new
_update_all_images
if the set of images in view has changed.transformers.pipeline
is not async, so we run that in subprocess viamultiprocess_predictor.py