-
Notifications
You must be signed in to change notification settings - Fork 0
KLAUS-256: Adding filters for gt data in compare-gt.py #44
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
dahhei
wants to merge
7
commits into
master
Choose a base branch
from
filter-gt
base: master
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 5 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
93d3dcf
adjusted for filtering gt AND pred
dahhei 3d20cd9
linting error fix
dahhei ae5b7f2
ruff linting
dahhei fdff842
more ruff linting
dahhei ca9b6e3
docstring update
dahhei 8125259
deleting redundant code
dahhei 71de9e0
trying to fix test outcome
dahhei 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
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
Oops, something went wrong.
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.
@SkepticRaven, I'm curious about your opinion on permanently setting this to
False. This is thefilter_ground_truthargument of thegenerate_iou_scancall.It's used to switch on this call to
filter_by_settings.Which, for reference, is defined here.
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.
From a machine learning perspective, we should avoid modifying the ground truth data. I'd prefer it wasn't a parameter at all. The reason this capability exists is just for practical reasons. While the user should go in and manually modify the ground truth data, it can get effort-expensive. Allowing the ground truth to be modify-able runs the risk of observing "improved" performance by removing shorter but difficult real events.
At least in this edit, the behavior appears to be generating an unfiltered and filtered version (edits below, L345).
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.
So does it make sense to remove the
filter_ground_truthargument entirely?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.
We should first look into what
generate_filtered_iou_curvedoes differently. That new function appears to have high overlap with what that arg does.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 it'd make more sense for me to change this back to filter_ground_truth and modify my code to work with some of the older logic. In the current state, I rewrote the whole function without removing the older functionality.