-
-
Notifications
You must be signed in to change notification settings - Fork 399
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
Add support for generic number type #3191
Conversation
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.
I think we're going to need to see a bunch of tests and examples. I don't know if I understand all the implications.
Yes, it's still a WIP but I opened it so that we can see that there is already one PR open with many changes all around so that we avoid making other big refactoring that will create conflicts |
8e649b6
to
ef85fe9
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #3191 +/- ##
==========================================
- Coverage 98.06% 98.05% -0.02%
==========================================
Files 34 34
Lines 4921 4981 +60
==========================================
+ Hits 4826 4884 +58
- Misses 95 97 +2
☔ View full report in Codecov by Sentry. |
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.
I haven't reviewed fully; this is a big PR. I'm a little nervous about making such a big change to JuMP at this point.
One intermediate option that I think we've discussed before is to take data as input in Float64 and to convert it to the type T
before passing to the solver. Then we get the benefit of solving in higher fidelity, whilst keeping JuMP simple.
There are a few advantages of supporting arbitrary types at the JuMP level:
I'd also argue that this PR keeps JuMP simple. There are a lot of changes but it's mostly renamings from |
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.
Heading in the right direction. I don't really know how we're going to test this in its entirety.
82ebff1
to
7f31702
Compare
7f31702
to
626a3f3
Compare
e4ae1c9
to
6a63f79
Compare
@odow Any ideas where the
are coming from ? |
7f9143a
to
1b2bfba
Compare
docs/src/tutorials/getting_started/getting_started_with_JuMP.jl
Outdated
Show resolved
Hide resolved
The other errors are because of
in |
ef9a889
to
fde1a3b
Compare
Thanks, all green now. Let's try to merge this now before any other PR as it has a tendency of conflicting with everything ^^ |
Here's the |
The PowerModels failure seems unrelated and just a numerical issue. But this is breaking for InfiniteOpt. |
Seems like InfiniteOpt also broken with #3350, so this might not be the cause. |
Co-authored-by: Oscar Dowson <[email protected]>
Co-authored-by: Oscar Dowson <[email protected]>
Co-authored-by: Oscar Dowson <[email protected]>
e59ea54
to
2408234
Compare
con::AbstractConstraint, | ||
name::String = "", | ||
) | ||
con = model_convert(model, con) |
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.
@blegat: it was sufficient to fix the extension in the docs by adding this here. This means some extra allocations though, so we should probably figure out when we don't need to model_convert
objects?
New run with more packages: https://github.com/jump-dev/JuMP.jl/actions/runs/5051033339
Nothing jumps out as breaking in the extension tests. What other packages should we test? I'm still nervous for such a large change to JuMP that's hard to revert or push out in small batches. |
So what if we split this up into a few different PRs?
Then 1, 2, and 3 are all definitely non-breaking. And we can focus on 4 as a source of the issue. |
solvers like Alpine |
Since it is a big change, we can even consider releasing a beta version and asking in discourse for people to report. |
Co-authored-by: Oscar Dowson <[email protected]>
Yes, we can definitely split the PR into a breaking and non-breaking part |
Am I brave enough to try rebasing this on top of #3378? |
See #3385 for a replacement. |
InfiniteOpt's master is now up to date and passing tests, should you want to retest. |
Closing in favor of #3385 |
A first approach could be to change
Model
intoModel{T}
withModel() = Model{Float64}()
andVariableRef
intoVariableRef{T}
withVariableRef() = VariableRef{Float64}()
.However, this would be breaking with respect to performance. Users having a
Model
orVariableRef
as a field would not have a concrete field anymore.Moreover, even if it is nonbreaking, users only caring about
Float64
will have non-concrete fields when usingModel
orVariableRef
even if these types seem concrete from the name.For these reasons, this PR is considering a different approach.
We define
GenericModel{T}
andconst Model = GenericModel{Float64}
andGenericVariableRef{T}
withconst VariableRef = GenericVariableRef{Float64}
.We now have a few advantages:
GenericModel
andGenericVariableRef
as astruct
field, it's clear from the name that it is not concreteAffExpr
/GenericAffExpr
andQuadExpr
/GenericQuadExpr
This PR also adds the
value_type
function to the JuMP API. This function gives the return type of theJuMP.value
function on variables of the JuMP model. It can be called on a variable or a model. Calling it on an affine or quadratic may be confusing since the coefficient of the expression might be different from thevalue_type
of the variables or even the return type ofvalue
on the expression. It is therefore not defined for affine and quadratic expressions.Note that incorporates the changes of
because otherwise, I would have had to complicate the tests because of this:
Here if how it looks like
Closes #2025