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

Fixes for Laplace, Logistic and Weibull (2) #303

Merged
merged 3 commits into from
Jul 8, 2024

Conversation

MichalLauer
Copy link
Contributor

Replacement for #302 , should be much better. Sorry!:)

Proposed changes

This PR fixes several bugs within distributions.

Types of changes

Put an x in the boxes that apply or remove the lines that don't.

  • Bugfix (non-breaking change which fixes an issue)

Checklist

Put an x in the boxes that apply or remove the lines that don't. This is a reminder of what we are going to look for before merging your code.

  • I have added unit tests and all tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added documentation where necessary
  • Any dependent changes have been merged and published in downstream modules

Further comments

Fixes the following errors:

# Laplace distribution
distr6::Laplace$new(var = 4)
# Logistic distribution
distr6::Logistic$new(sd = 1)$getParameterValue('mean')
distr6::Logistic$new(sd = 1)$getParameterValue('scale')
distr6::Logistic$new(sd = 1)$getParameterValue('sd')
# Weibull distribution
distr6::Weibull$new(altscale = 3)$getParameterValue("shape")
distr6::Weibull$new(altscale = 3)$getParameterValue("scale")
distr6::Weibull$new(altscale = 3)$getParameterValue("altscale")

@RaphaelS1
Copy link
Collaborator

aha so the order of variables was the wrong way around?

@RaphaelS1
Copy link
Collaborator

Would you mind just running a reprex before and after the error so I can see the fix? (sorry on a computer that doesn't play nicely with R)

@MichalLauer
Copy link
Contributor Author

Sure,

Before fix:

# Laplace distribution
distr6::Laplace$new(var = 4)
#> Error in vars/2: non-numeric argument to binary operator
# Logistic distribution
distr6::Logistic$new(sd = 1)$getParameterValue('mean')
#> Error in expand_list(self$ids, x): ids in 'names' not in 'named_var'
distr6::Logistic$new(sd = 1)$getParameterValue('scale')
#> Error in expand_list(self$ids, x): ids in 'names' not in 'named_var'
distr6::Logistic$new(sd = 1)$getParameterValue('sd')
#> Error in expand_list(self$ids, x): ids in 'names' not in 'named_var'
# Weibull distribution
distr6::Weibull$new(altscale = 3)$getParameterValue("shape")
#> Error in expand_list(self$ids, x): ids in 'names' not in 'named_var'
distr6::Weibull$new(altscale = 3)$getParameterValue("scale")
#> Error in expand_list(self$ids, x): ids in 'names' not in 'named_var'
distr6::Weibull$new(altscale = 3)$getParameterValue("altscale")
#> Error in expand_list(self$ids, x): ids in 'names' not in 'named_var'

After fix:

# Laplace distribution
distr6::Laplace$new(var = 4)
#> Lap(mean = 0, var = 4)
# Logistic distribution
distr6::Logistic$new(sd = 1)$getParameterValue('mean')
#> [1] 0
distr6::Logistic$new(sd = 1)$getParameterValue('scale')
#> [1] 0.5513289
distr6::Logistic$new(sd = 1)$getParameterValue('sd')
#> [1] 1
# Weibull distribution
distr6::Weibull$new(altscale = 3)$getParameterValue("shape")
#> [1] 1
distr6::Weibull$new(altscale = 3)$getParameterValue("scale")
#> [1] 0.3333333
distr6::Weibull$new(altscale = 3)$getParameterValue("altscale")
#> [1] 3

@RaphaelS1
Copy link
Collaborator

Thanks! That's weird previous checks passed ... But good to see it's a breaking error and not a silent one

@RaphaelS1 RaphaelS1 merged commit a642cd3 into xoopR:main Jul 8, 2024
1 check failed
@MichalLauer MichalLauer deleted the param-fixes branch July 8, 2024 13:52
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