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.

Sign up for GitHub

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

Jump to bottom

simplify and improve type intersection algorithm a bit #41795

Merged
merged 2 commits into from
Aug 26, 2021
Merged

simplify and improve type intersection algorithm a bit #41795

merged 2 commits into from
Aug 26, 2021

Conversation

JeffBezanson
Copy link
Member

This removes some code and makes a class of results more conservative, fixing some potential cases of unsoundness.

I stumbled into this while working on #41738. It doesn't fix that issue yet but should make it easier, and was a nice discovery in the process. See the bottom of the diff for a changed test case where I'm pretty sure we were getting the wrong answer before. Let's see if this passes CI.

@JeffBezanson JeffBezanson added the types and dispatch Types, subtyping and method dispatch label Aug 5, 2021
@JeffBezanson
Copy link
Member Author

@nanosoldier runtests(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

martinholters
martinholters reviewed Aug 6, 2021
View reviewed changes
test/subtype.jl Outdated Show resolved Hide resolved
@JeffBezanson
Copy link
Member Author

Looks like there are some new stack overflows in package tests.

@JeffBezanson JeffBezanson force-pushed the jb/simpler_intersection branch 2 times, most recently from cbafa8d to 3fbf115 Compare August 7, 2021 22:33
@JeffBezanson
Copy link
Member Author

At this point I have a change that just barely fixes #41738, by managing to return a non-concrete type, but one that is still too narrow. There are a couple minor test changes that I think are reasonable. I have an idea for how to get a more correct result, but we may be able to use this in a pinch.

@JeffBezanson JeffBezanson force-pushed the jb/simpler_intersection branch 2 times, most recently from dca187d to f3ec252 Compare August 9, 2021 15:04
@JeffBezanson
Copy link
Member Author

@nanosoldier runtests(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@JeffBezanson
Copy link
Member Author

Stack overflow in ValkyrieRobot.

@JeffBezanson
Copy link
Member Author

@nanosoldier runtests(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@JeffBezanson JeffBezanson mentioned this pull request Aug 11, 2021
Merged
@JeffBezanson JeffBezanson force-pushed the jb/simpler_intersection branch 3 times, most recently from 2869088 to fcdb63d Compare August 12, 2021 17:08
@JeffBezanson
Copy link
Member Author

@nanosoldier runtests(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@JeffBezanson
Copy link
Member Author

SumOfSquares has an unreachable-reached; other than that we're getting there.

@JeffBezanson JeffBezanson force-pushed the jb/simpler_intersection branch 2 times, most recently from 0af2412 to e86099b Compare August 15, 2021 17:58
@JeffBezanson
Copy link
Member Author

@nanosoldier runtests(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@JeffBezanson
Copy link
Member Author

@nanosoldier runtests(["AdvancedVI", "BifurcationInference", "BifurcationKit", "BlackBoxOptim", "Causal", "ConstructionBase", "Dance", "DelayDiffEq", "DiffEqParamEstim", "DynamicBoundsBase", "Expect", "FMIFlux", "ForwardDiff", "LazyAlgebra", "MCPhyloTree", "Minio", "ODE", "PowerGraphics", "RemoveLFS", "SalesForceBulkApi", "SampledSignals", "SpatialJackknife", "StochasticRounding", "SymbolicRegression", "YAActL"], vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@JeffBezanson
Copy link
Member Author

@nanosoldier runtests(["AdvancedVI", "BifurcationInference", "BifurcationKit", "BlackBoxOptim", "Causal", "ConstructionBase", "Dance", "DelayDiffEq", "DiffEqParamEstim", "DynamicBoundsBase", "Expect", "FMIFlux", "ForwardDiff", "LazyAlgebra", "MCPhyloTree", "Minio", "ODE", "PowerGraphics", "RemoveLFS", "SalesForceBulkApi", "SampledSignals", "SpatialJackknife", "StochasticRounding", "SymbolicRegression", "YAActL"], vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@JeffBezanson
Copy link
Member Author

The remaining problem seems to be ConstructionBase, where intersection is hanging or possibly just taking a very long time.

@JeffBezanson
Copy link
Member Author

The slowdown happens with just b06f813, to narrow it down a bit.

@JeffBezanson JeffBezanson mentioned this pull request Aug 20, 2021
Closed
vtjnash
vtjnash reviewed Aug 20, 2021
View reviewed changes
base/compiler/utilities.jl Outdated Show resolved Hide resolved
@JeffBezanson JeffBezanson force-pushed the jb/simpler_intersection branch 2 times, most recently from e3e550e to 0a1bf84 Compare August 23, 2021 21:04
@JeffBezanson
Copy link
Member Author

@nanosoldier runtests(["Causal", "ConstructionBase", "DiffEqParamEstim", "DynamicBoundsBase", "FMIFlux", "SpatialJackknife"], vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - no new issues were detected. A full report can be found here.

@JeffBezanson
Copy link
Member Author

@nanosoldier runtests(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@JeffBezanson
Copy link
Member Author

Looking good. Just to double check:

@nanosoldier runtests(["CMAEvolutionStrategy", "ForwardDiff", "IntervalTrees", "JetPackDSP", "LoopThrottle", "MatrixNetworks", "SparseRegression", "SpatialJackknife", "Tectonic"], vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - no new issues were detected. A full report can be found here.

@JeffBezanson
Copy link
Member Author

Note: requires #41867 and #41976

@vtjnash
Copy link
Member

vtjnash commented Aug 25, 2021

both are merged now. rebase?

@JeffBezanson
simplify and improve type intersection algorithm a bit
13bcdf1 
This removes some code and makes a class of results more conservative,
fixing some potential cases of unsoundness.
@JeffBezanson
give wider/safer intersection result for vars used in both invariant …
71757cd 
…and covariant position

fixes #41738
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Aug 25, 2021
@JeffBezanson JeffBezanson merged commit 1bbba21 into master Aug 26, 2021
@JeffBezanson JeffBezanson deleted the jb/simpler_intersection branch August 26, 2021 18:43
@palday
Copy link
Contributor

palday commented Aug 27, 2021

Should this be backported to 1.7 as part of the fix for #40048 ?

@KristofferC KristofferC mentioned this pull request Aug 27, 2021
Merged
63 tasks
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Sep 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash approved these changes

@martinholters martinholters martinholters left review comments

Assignees
No one assigned
Labels
types and dispatch Types, subtyping and method dispatch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@JeffBezanson @nanosoldier @vtjnash @palday @martinholters @KristofferC @DilumAluthge