Closed MilesCranmer closed 1 year ago
@MilesCranmer This looks like a great contribution, thank you. Appreciate the work and excellent presentation.
Because this package is sits so low in the MLJ ecosystem, let's run some tests from MLJTestIntegration.jl, just to minimize the chance of surprises. @OkonSamuel do you have some time to verify the following?
(Sometimes when I run these very extensive tests, I find new unrelated issues. In those cases I just remove appropriate models from tests in the notebook, add links in the notebook to the issues blocking inclusion, and update the notebook on the MLJTestIntegration#dev
branch. We should automate this, I know, but haven't got around to it yet.)
nice catch @MilesCranmer
@OkonSamuel Any progress on integration tests?
The integration tests are good.
Fixes #174.
The bug was due to
length(default.args) > 1
being used to check whether a constraint was present in the default expression. What should actually be done isdefault.head == :(::)
.Using
length(default.args)
is incorrect because some defaults can have multiple args, such asLikewise, the user might not have used
::
at all to define the constraint.This PR checks for the
::
operator to identify a constraint which fixes this issue.Note that this fix is backwards compatible, so long as people have not been exploiting the bug. See my note at the end of this post for more details.
With this change, we can now do:
whereas before you would get an error due to the incorrectly parsed default, which would parse
0
as the default and1
as the constraint:cc @ablaom
What changes is that a user can no longer exploit the bug. For example, before you actually do:
to give a constraint. Any expression which has the constraint in the second argument would be a valid way of defining this model. Unless you are aware of anybody doing this, I wouldn't worry about it though.
Now if you try to use that syntax, you would get:
which is what we want.