-
Notifications
You must be signed in to change notification settings - Fork 171
Add measure property to PDAHypothesiser #1204
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
Open
rohatgunes
wants to merge
2
commits into
dstl:main
Choose a base branch
from
rohatgunes:rgunes/pda_measure_property
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Wondering about what measures you are thinking of using, as here you can see the gate threshold it's compared to is designed to be in the same units as the measure?
Uh oh!
There was an error while loading. Please reload this page.
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.
The gate threshold is using the gating-probability value to generate a N-dimensional Chi-square scalar distance value as threshold.
We think this can be compared to N-dimensional distance measurements like Mahalanobis, Euclidean or Weighted Euclidean. Also, during Extended Kalman operation, the innovation covariance can slightly deviate from positive semi-definite form which leads to Mahalanobis measure giving misleading output because of square-root of negative number. Having a customizable measure function can help to experiment with others methods.
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.
@sdhiscocks , was that a clear answer? Please let me know.
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.
Sorry for delay @rohatgunes.
I'm still not clear how Euclidean for example would work in comparison with Chi-square distance. But I can see that other gating approaches may be useful.
Maybe it would also be useful to be able to modify the
_gate_thresholdfunction in some way? Or replace the entire if line with a call to a function which returns true or false if it is inside or outside the gate?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.
It is worth mentioning here that we also have the Gater class, and more specifically the DistanceGater that should allow users to specify a custom measure for gating with any Hypothesiser.
Uh oh!
There was an error while loading. Please reload this page.
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.
Hmm,
DistanceGaterdoes not seem to be compatible withPDAHypothesiseras is, where the gating affects probability of a particular hypothesis. It would be possible to keepPDAHypothesiserlegacy behaviour intact and letPDAHypothesiserstill be used standalone, and only treat detections as gated if aProbabilityGateris to be used as a wrapper. Maybe this is too defensive/lazy of a programming but otherwise the cake does not seem to be worth the candle? Considering all references to be updated in docs etc. as well as keeping standalone option intact.So I thought of something like below, to further allow
_validation_region_volume()take its course withinPDAHypothesiser. What do you think @sglvladi ? Would this be a desired approach @sdhiscocks ? Or would you prefer a different overhaul to override gating calculation within or keep_gate_thresholdand wrapper threshold as 2-layer gating like inDistanceHypothesiserwheremissed_distanceis the 1st layer andGateradds 2nd?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.
tl;dr:
Options:
Gaterimplementation, complex to respect legacy but followsGaterclass.measureand_gate_thresholdasPropertywith defaults to respect legacy, simple, straightforward.Which one will it be?