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

Feature/metrics/stable metrics #382

Closed

Conversation

dapladoc
Copy link
Collaborator

@dapladoc dapladoc commented Jun 4, 2023

Introduce permutation stable version of the retrieval metrics. The idea is to sort gt_tops for which distances are the same, i.e. if distances_tops and gt_tops are

distances_tops = [1, 2, 2, 2, 3]
gt_tops = [0, 1, 0, 1, 0]

then we need to obtain

gt_tops = [0, 0, 1, 1, 0]
#             -------
#             sort gt_tops where there are multiple
#             distances of the same value

The code for the retrieval metrics itself doesn't change.

@AlekseySh AlekseySh linked an issue Jun 4, 2023 that may be closed by this pull request
@dapladoc
Copy link
Collaborator Author

dapladoc commented Jun 5, 2023

In the failed test called test_metrics_is_similar_in_ddp it is said that

Metrics shouldn't be equal, but very similar

If I change the atol for the test from 5e-3 to 8e-3 the test passes. Is this workaround ok?

@AlekseySh
Copy link
Contributor

@dapladoc changing tolerance is okay

@dapladoc dapladoc changed the title Feature/metrics/stable metrics [WIP] Feature/metrics/stable metrics Jun 7, 2023
@dapladoc
Copy link
Collaborator Author

dapladoc commented Jun 7, 2023

@AlekseySh @DaloroAT hey guys. It looks like this PR is ready to be reviewed.

As I described above the main idea is to sort gt_tops values corresponding to the same distance values. In order to do it properly, first we need to find new value of max_k such that torch.any(distances[:, max_k] == distances[:, max_k + 1] is False (function _estimate_suitable_max_k). Next, we need to make the sorting (function _sort_gt_tops). And all the steps for getting the gt_tops is now encapsulated in function _get_gt_tops.

@dapladoc dapladoc changed the title [WIP] Feature/metrics/stable metrics Feature/metrics/stable metrics Jun 7, 2023
@AlekseySh
Copy link
Contributor

@DaloroAT What do you think? Does bringing the additional logic worth the benefits of metrics' stability?
PS. Initially, I thought that the required extra logic would be smaller and simpler...

@AlekseySh
Copy link
Contributor

@dapladoc let's implement what we've decided offline:

  1. measure overhead
  2. introduce a flag for this functionality available for developers only (so, we don't expose is to config)
  3. depends on the first step, set this flag as True or False by default

@AlekseySh
Copy link
Contributor

@dapladoc hey! just a reminder :)

@AlekseySh
Copy link
Contributor

AlekseySh commented Jun 7, 2024

Let me close it for now due to inactivity.

Metrics code has also been significantly modified.
The order of predictions is determined in RetrievalResults, so, I'm not sure this problem is even actual now.

So, anyway, the PR has to be reworked.

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.

Metric value may slightly randomly change
2 participants