Refactor/detections fetching#618
Conversation
subdavis
force-pushed
the
refactor/detections-fetching
branch
from
3320ca2 to
1fe4c02
Compare
BryonLewis
left a comment
BryonLewis
left a comment
There was a problem hiding this comment.
couple items to look at
| item = detections_item(folder, strict) | ||
| if item is None and not strict: | ||
| return None | ||
| return Item().childFiles(item[0])[0] |
There was a problem hiding this comment.
in detections_item() you already return all_items[0] so I think this might be an index into something that doesn't exist?
| all_items = all_detections_items(folder) | ||
| detection = all_items[0] if all_items else None |
There was a problem hiding this comment.
all_detection_items can return an empty Cursor object which is still true. This is the case if you upload media with no source csv detections. This could also be the case if the detections were removed from the girder folder manually so there was no return value.
something like all_items[0] if all_items.count() > 0 else None may work?
subdavis
force-pushed
the
refactor/detections-fetching
branch
from
07ca748 to
92eb4fb
Compare
BryonLewis
left a comment
BryonLewis
left a comment
There was a problem hiding this comment.
Very minor thing with variable name.
| detections_json_item = detections_item(folder, strict=True) | ||
| ensure_csv_detections_file(folder, detections_json_item, user) | ||
| detection_list.append(detections_json_item) |
There was a problem hiding this comment.
Little confusing at first glance because we are using a json named ITEM to store both the json and the csv. This detection_json_item could already have an attached csv. I don't believe we assigned detection metadata to any csv's anymore so all detection_items at least have a json file? Maybe favor using detection_item here?
There was a problem hiding this comment.
I did this just because the name of the function was so similar to the variable that it was a readability issue. Changed to train_on_detections_item, is that OK?
There was a problem hiding this comment.
Yeah I completely forgot that would be a similar name to the function.
2 participants
Refactor the sorta messy girder queries for detection items because they're getting copy-pasted all over.
Don't really want to block #590, so will rebase on main after it merges.