You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
[loud thinking / suggestion] Maybe I do not get the full context here, but if we are converting inference results to sv detections, then maybe we could get rid of parametrisation of fields names that are related to responses in favour of constants (or even get rid of operations on dicts in favour of operating on responses objects, but then what with QR codes and barcodes). Then we could also name the input parameter as batch_inference_results and just return list[sv.Detections] without embedding that into dict["predictions"] = [<sv.Detections>]. That would shift the responsibility of forming proper outputs of blocks into caller which owns this. Obviously that would cost another round of for-loop on the caller side, but what is visible in barcode detection - the fact that other keys that predictions_key are forgotten in output list of dicts actually makes this function harder to read and understand in global context.
Follow-up of code review comment:
[loud thinking / suggestion] Maybe I do not get the full context here, but if we are converting inference results to sv detections, then maybe we could get rid of parametrisation of fields names that are related to responses in favour of constants (or even get rid of operations on dicts in favour of operating on responses objects, but then what with QR codes and barcodes). Then we could also name the input parameter as
batch_inference_results
and just returnlist[sv.Detections]
without embedding that intodict["predictions"] = [<sv.Detections>]
. That would shift the responsibility of forming proper outputs of blocks into caller which owns this. Obviously that would cost another round of for-loop on the caller side, but what is visible in barcode detection - the fact that other keys thatpredictions_key
are forgotten in output list of dicts actually makes this function harder to read and understand in global context.Originally posted by @PawelPeczek-Roboflow in #392 (comment)
The text was updated successfully, but these errors were encountered: