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

ArgumentError: Paramters must be greater than zero. [sic] #655

Open
lrworth opened this issue Mar 26, 2021 · 4 comments
Open

ArgumentError: Paramters must be greater than zero. [sic] #655

lrworth opened this issue Mar 26, 2021 · 4 comments
Assignees

Comments

@lrworth
Copy link

lrworth commented Mar 26, 2021

Describe the bug
In split-4.0.0.pre, the dashboard page fails with an exception due to splitrb passing invalid parameters to BetaDistribution.

To Reproduce
I haven't got a small reproduction case, but it is obvious what is happening. Within Split::Experiment#calc_beta_params:

  1. conversions == 1.
  2. We have some alternative where alternative.participant_count == 0.
  3. Under these conditions:
  • alpha = 1 + conversions so alpha == 1 + 1 == 2.
  • beta = 1 + alternative.participant_count - conversions so beta == 1 + 0 - 1 == 0
  1. In #calc_simulated_conversion_rates, beta is passed to RubyStats::BetaDistribution.
  2. BetaDistribution#initialize requires beta > 0 and raises an exception.

Assuming there is no bug in the logic here, a good resolution might be to only call experiment.calc_winning_alternatives within dashboard/views/_experiment.erb if it will succeed.

@lrworth lrworth changed the title ArgumentError: Paramters must be greater than zero. ArgumentError: Paramters must be greater than zero. [sic] Mar 26, 2021
@lrworth
Copy link
Author

lrworth commented Mar 26, 2021

I get a very similar problem with split-3.4.1. Perhaps I'm using it incorrectly?

@lrworth
Copy link
Author

lrworth commented Mar 26, 2021

Probably related to #563.

@andrehjr
Copy link
Member

Hi there, yeah, it's probably related to that. BetaDistribution requires > 0.

As the participation_count should not be less than the completed count. At first, the solution was to ensure the participation_count was also increased when someone forced that alternative. But then I reworked that on #637 in order to not take admin into metrics. So, it should not be possible to get into the same problem on v4.

But that still leaves a problem where the counts on alternatives may already be wrong.

I can make a patch to avoid running the BetaDistribution in such situations and think about how I can warn users about this on the dashboard.

For now, you could alternative.participant_count = completion_count for the alternatives where the 'conversions' is greater than that. WDYT?

@lrworth
Copy link
Author

lrworth commented Mar 28, 2021

Interesting. I’m not sure how I got my database into such a state but clearing Redis seems to have done the trick.

If it’s always the case that participation_count >= completion_count then I’d personally prefer some kind of error describing which “impossible” thing happened so that I could fix it. But as you seem to have made this situation far less likely, it might not be worth the effort. Perhaps a guard and error log surrounding the call to BetaDistribution will suffice? If so I’m happy to provide a PR.

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

No branches or pull requests

2 participants