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

bug: make sampler work when NaN values are predicted by the models #18

Merged
merged 2 commits into from
Sep 17, 2023

Conversation

younesStrittmatter
Copy link
Contributor

Sampler was breaking with an Error, if one of the models predicts NaN or Inf for a single condition.

Fix:
The disagrement (distance) between two models get's set to 0 if at least one of them predict NaN or Inf at that condition.

Rationale:

  • Throwing an Error is no good, since it breaks with a lot of models
  • Dropping the condition is no good, since some models would "clog" the sampler (For example, if a model is sqrt(x), this means no negative conditions get sampled anymore.
  • Setting the distance to a high number would lead to overly sampling potentially uninformative conditions (especially if the ground truth actually is undefined at these condition)
    -> Setting the distance to zero doesn't encourage sampling this condition, but also doesn't make it impossible.

Copy link
Contributor

@musslick musslick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Only consideration: Do we want to issue a warning once a nan is detected, so that the user is informed about how the disagreement is computed in this case?

Copy link
Contributor

@musslick musslick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@younesStrittmatter younesStrittmatter merged commit 6dd86dc into main Sep 17, 2023
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.

2 participants