Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this still cause some invalidations ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing serious, if memory serves. The real problem is totally-unconstrained
T
.When fixing invalidations, usually I don't like to suggest restrictions on what packages can do (better to fix the code being invalidated). But
convert
is one of those rare functions that's used in a ton of places with poor inference on the 2nd argument, and it's often used to make inference better. Consequently it's bad to specialize only on the 2nd argument, since so many callers don't know what type their 2nd argument is. It's generally fine if you specialize on the first argument.You can add more
convert
methods than this, I just gave one that is needed to pass tests. But keep in mind that the call site could also just callscalarize
directly, and that would be the cleanest solution of all (just deleting this method altogether).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks for the detailed explanation. I try to keep the same behavior independently of whether the coefficient is a number or not as the coefficient is for instance sometimes a JuMP expressions which is not a number. Therefore I might remove this
convert
and see what's break. Meanwhile I'll merge this PR. Thanks!