Skip to content
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

Fixing val.py incorrect recalls issue #43 #44

Merged
merged 3 commits into from
Nov 11, 2021
Merged

Conversation

StephenHausler
Copy link
Contributor

Made changes to val.py that should fix the indices issue that was causing low reported recalls for mapillary validation on the cph and sf datasets. Without PCA, the recalls with NetVLAD (with the mapillary trained model) are now: 0.495, 0.65, 0.718, 0.77, 0.83, 0.868.

@Tobias-Fischer
Copy link
Contributor

Instead of hardcoding, can't we use eval_set.qIdx and eval_set.pidx?

@Tobias-Fischer
Copy link
Contributor

Also, what are the recalls for Patch-NetVLAD (compared to vanilla NetVLAD)?

@StephenHausler
Copy link
Contributor Author

Instead of hardcoding, can't we use eval_set.qIdx and eval_set.pidx?

I don't think so, since msls concatenates the two datasets together. So eval_set.qIdx contains a list of query indices that are from both datasets.

@StephenHausler
Copy link
Contributor Author

I've made changes that now means that val.py should work in the general case, with any combination of mapillary cities. Please re-review and let me know your feedback.

@Tobias-Fischer
Copy link
Contributor

Tobias-Fischer commented Nov 10, 2021

Thanks, lgtm! What do you think @oravus?

@oravus
Copy link
Contributor

oravus commented Nov 10, 2021

Yep, this should do (ignore my earlier comment).
EDIT: Re-opened

@oravus oravus closed this Nov 10, 2021
@Tobias-Fischer
Copy link
Contributor

Agreed that it's not very nice atm. I've got a plan, let me fix tomorrow

@oravus oravus reopened this Nov 10, 2021
@Tobias-Fischer
Copy link
Contributor

Fix #43

@Tobias-Fischer
Copy link
Contributor

Hey @StephenHausler @oravus - I think f06d664 makes it a bit cleaner. I did "open loop" coding though and haven't tested if it still works properly - could you please check @StephenHausler?

@StephenHausler
Copy link
Contributor Author

I've just tested the code, it works perfectly. Thanks Tobi for your refactor, looks great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants