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

Implementing #57: Real coefficients #58

Merged
merged 9 commits into from Jan 16, 2020
Merged

Conversation

Wikunia
Copy link
Owner

@Wikunia Wikunia commented Jan 13, 2020

First go of implementing real coefficients.

  • Check everything again
  • Check that there is no performance regression
  • Is it reasonable to support Rational weights in a different way?

@codecov
Copy link

codecov bot commented Jan 13, 2020

Codecov Report

Merging #58 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #58   +/-   ##
=======================================
  Coverage   98.93%   98.93%           
=======================================
  Files          20       20           
  Lines        1407     1407           
=======================================
  Hits         1392     1392           
  Misses         15       15

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db86e58...cc899fd. Read the comment docs.

@matbesancon
Copy link
Contributor

To stay efficient when using non-concrete types, better parameterize with a generic:

mutable struct LinearVariables{T <: Real}
    indices             :: Vector{Int}
    coeffs              ::  Vector{T}
end 

Instead of Vector{Real}

@matbesancon
Copy link
Contributor

When creating a LinearConstraint, either promote every real to the same type, or use different types for the different field types:

    mins                :: Vector{TM1}
    maxs                :: Vector{TM2}
    pre_mins            :: Vector{TP1}
    pre_maxs            :: Vector{TP1l} 
  # etc

test/runtests.jl Outdated Show resolved Hide resolved
src/util.jl Outdated Show resolved Hide resolved
@Wikunia
Copy link
Owner Author

Wikunia commented Jan 14, 2020

Thanks for having a look:
I only need the linear variables if JuMP isn't used as the modelling layer, therefore I'm not sure whether it's reasonable to longer support that anyway. Maybe you have some thoughts on that. If JuMP is used all variables in a constraint will have the same type which is currently: Float64 anyway.

@matbesancon
Copy link
Contributor

LinearVariables might be a tricky name, maybe LinearCombination, or LinearFunction? I think you should not hard-code Float64, JuMP will eventually get generic numerical types: jump-dev/JuMP.jl#2025
Although it's not on the immediate roadmap I believe

src/options.jl Outdated Show resolved Hide resolved
@Wikunia Wikunia changed the title Implementing #57 Implementing #57: Real coefficients Jan 14, 2020
@matbesancon
Copy link
Contributor

I only need the linear variables if JuMP isn't used as the modelling layer

I think it is worth keeping, given the mention above that JuMP does not yet support generic numeric types

@Wikunia
Copy link
Owner Author

Wikunia commented Jan 14, 2020

When creating a LinearConstraint, either promote every real to the same type, or use different types for the different field types:

What is the easiest way to get the common type of values?
Thought Julia would do that automatically but looks like it doesn't.

@matbesancon
Copy link
Contributor

What is the easiest way to get the common type of values?

# on values
julia> promote(3, false)
(3, 0)

# on types
julia> promote_type(Float32, Float64)
Float64

@Wikunia
Copy link
Owner Author

Wikunia commented Jan 14, 2020

Implemented:

  • LinearVariables -> LinearCombination
  • Generic type for LinearCombination

@codecov
Copy link

codecov bot commented Jan 16, 2020

Codecov Report

Merging #58 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #58   +/-   ##
=======================================
  Coverage   98.93%   98.93%           
=======================================
  Files          20       20           
  Lines        1407     1407           
=======================================
  Hits         1392     1392           
  Misses         15       15
Impacted Files Coverage Δ
src/ConstraintSolver.jl 99.12% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db86e58...7c32e07. Read the comment docs.

@Wikunia
Copy link
Owner Author

Wikunia commented Jan 16, 2020

Checked that there are no changes compared to master for killer sudoku regarding number of steps. The runtime might be slightly worse up but not significantly.

@Wikunia
Copy link
Owner Author

Wikunia commented Jan 16, 2020

If there are no other things @matbesancon I would merge this.

@matbesancon
Copy link
Contributor

Yup let's go with it :)

@matbesancon
Copy link
Contributor

anyway tests are passing, future things to do can be in other PRs

@Wikunia Wikunia merged commit eba2486 into master Jan 16, 2020
@Wikunia Wikunia deleted the feature-real-coefficients branch January 16, 2020 16:36
@Wikunia Wikunia restored the feature-real-coefficients branch January 19, 2020 21:09
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.

None yet

2 participants