-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Detections.empty()
invalidates detections, causes crashes when Detections.merge()
is called.
#943
Comments
* Addresses issue roboflow#928 matching implementation in ByteTrack, but sidestepping the underlying cause outlined in roboflow#943.
Affected codeThe good news is - the error mostly appears when Affected functions:
See Colab for more details. |
Mitigation OptionsTL;DR: I think Option 3 is the least-bad solution. Adds complexity in exactly one location, keeps design and architecture intact, stops errors, prevents future worries. I also advocate for two eventual changes regardless of option taken (final section) Here's how I see various mitigation options and their downsides. I can only speculate about the design goals and future plans of Roboflow, and I do not have access to the internal message board. So my goal now is to bring other devs up to speed and help think through the solutions. My Assumptions
Caveats
Option 1: Do nothingThe error has two stages:
Maybe it's not worth fixing? There haven't been many complaints. The devs just need to be extra careful of using Option 2: Forbid everything affected.(The worst approach, in my opinion) Option 3: Change merge.I like how If ( (Brainstorming) When we need new values for:
Option 4: Explicitly define what's being initialized.
Let's make it initialize ( Then, either pass an egument to Did you spot an issue? Suppose you have an intermediary function. A call chain of: Which type of That, or you need to forbid intermediaries from accepting Here's a thought experiment: try inlining Option 4b: Frickin' TemplatesWe're passing the type information with function calls, elsewhere that would be done via templates or generics. Type parameter syntaxis only introduced in 3.12. Perhaps TypeVar, could help, but generally this would make the code less accessible to new devs. Option 5: Multiple ClassesDid you notice that we're approaching a multi-class solution? My claim is that An option is to change the core design, split them in two, then define strict non-nullable members, adjust everything else to match that.
Downside:
Option 6: Get rid of nullable typesThis is my least thought-out option. Null values serve a purpose, representing invalid members - E.g. "This Perhaps I'm think so. Null checks can be removed in places like Essential ChangesI advocate for these two changes to be made eventually.
|
We've now implemented option 3 in #1177 Ideally, I'd like to see an attempt at trimming down Would that cause issues? Would that be easy fine? Would that entail too much work to be worth it? I'd like to know that. |
Search before asking
This is the underlying reason for #928. I believe it is important enough to deserve a separate issue.
Bug
Detectons.empty()
creates a very specific type ofDetections
, which is guaranteed to be incompatible with some models when merging results.Example
Suppose we run an instance segmentation model on a video. It finds one detection in frame 1, but no detections in frame 2. When we try calling
Detections.merge
on these detections, it raises an error.Error type 1
Error type 2
Deeper explanation
Detections
contains these variables:Suppose we call
Detections.merge([detections_1, detections_2])
. An error is rightfully raised when the same variable is defined differently in the detections. For example - whenmask
isNone
indetections_1
andnp.array([])
indetections_2
. It makes sense as we don't want to merge incompatible detections.However, when calling
Detections.empty()
, frequently used to initialize the no-detections case, it sets a specific subset of the fields - only thexyxy
,condifence
andclass_id
.Many models use other fields as well. Because of this, the set of defined fields in an empty detection might be different to what the model returns. When trying to merge these, an error is raised.
Environment
No response
Minimal Reproducible Example
See aforementioned example.
this Colab contains examples, list of functions affected.
Are you willing to submit a PR?
I could help submit a PR when I figure out a solution, but this is a non-trivial bug and to my best estimates, solving it 'correctly' would require a redesign of
Detections
. Quick remediation may mean band-aid code and hope that future features are written defensively whenmerge
is involved.The text was updated successfully, but these errors were encountered: