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

Enable multi-gpu AnomalyScoreThreshold #1413

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Seanny123
Copy link
Contributor

@Seanny123 Seanny123 commented Oct 13, 2023

Description

compute is not supposed to update state variables, in this case value, as defined by self.add_state("value", default=torch.tensor(default_value), persistent=True). The current implementation doesn't cause problems until you use multiple GPUs, where any changes to self.value in compute are discarded. Updating the variable is supposed to happen in update() or outside of the class. I elected to do it outside of the class.

Given how awkward this was to figure out and the hacky fix required, maybe AnomalyScoreThreshold shouldn't sub-class what its currently sub-classing?

Checklist

Ensure that you followed the following
  • I have performed a self-review of my code

@samet-akcay
Copy link
Contributor

@ashwinvaidya17 and @djdameln, you guys are expert at this. Can you check this out?

@Seanny123
Copy link
Contributor Author

As discussed in the linked issue, this seems to hurt model accuracy for some reason? So maybe don't merge.

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.

[Bug]: AnomalyScoreThreshold is incompatible with multi-GPU training
2 participants