Fixes for `Laplace`, `Logistic` and `Weibull` (2)

MichalLauer commented 4 months ago

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.

[x] 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
[x] I have added documentation where necessary
[x] 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 commented 4 months ago

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

RaphaelS1 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

xoopR / distr6