Skip to content

[Utilities] fix ProductOfSets to handle zero-dimensional rows#3022

Merged
odow merged 8 commits into
masterfrom
od/fix-product-of-sets
Jul 23, 2026
Merged

[Utilities] fix ProductOfSets to handle zero-dimensional rows#3022
odow merged 8 commits into
masterfrom
od/fix-product-of-sets

Conversation

@odow

@odow odow commented Jul 8, 2026

Copy link
Copy Markdown
Member

Closes #3019

@odow

odow commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

Okay, I'm happy with this. It's just simpler all round. It now has one UnitRange{Int} for each constraint. No more dictionary lookups, and we handle zero-dimensional rows.

VAF = MOI.VectorAffineFunction{Float64}
indices = MOI.get(sets, MOI.ListOfConstraintIndices{VAF,S}())
@test indices == MOI.ConstraintIndex{VAF,S}.([1, 3, 6, 8])
@test indices == MOI.ConstraintIndex{VAF,S}.([1, 2, 3, 4])

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The downside is that you might argue this is breaking, because we change the constraint indices. But I don't think we ever document that the index values are part of the public API. We might re-order them in any way. They just need to make sure that we iterate over them in the order that they were added (within a constraint type).

Comment thread src/Utilities/product_of_sets.jl Outdated
@odow

odow commented Jul 15, 2026

Copy link
Copy Markdown
Member Author

@odow

odow commented Jul 15, 2026

Copy link
Copy Markdown
Member Author

@odow

odow commented Jul 15, 2026

Copy link
Copy Markdown
Member Author

@odow

odow commented Jul 15, 2026

Copy link
Copy Markdown
Member Author

We can fix SCS, but I've also added a backward compatible getproperty call to this PR.

https://github.com/jump-dev/MathOptInterface.jl/actions/runs/29459997140

@odow

odow commented Jul 16, 2026

Copy link
Copy Markdown
Member Author

Sigh.

DiffOpt has: https://github.com/jump-dev/DiffOpt.jl/blob/master/src/product_of_sets.jl

This is why we shouldn't have all these tricky little helpers everywhere. Consumers use them in all sorts of unexpected ways and we get boxed into how we can support them.

@odow

odow commented Jul 16, 2026

Copy link
Copy Markdown
Member Author

@blegat we should chat about this. I don't really know how to get these simplifications in MOI without breaking DiffOpt.

We probably need to keep the current fields and just add some additional bookkeeping for the zero-dim rows. And then also go and fix this in DiffOpt.

@odow

odow commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

We probably need to keep the current fields and just add some additional bookkeeping for the zero-dim rows. And then also go and fix this in DiffOpt.

But that won't work because DiffOpt won't have those fields...

I've opened a PR in DiffOpt to remove the usage: jump-dev/DiffOpt.jl#372

@odow
odow marked this pull request as ready for review July 23, 2026 21:15
@odow
odow merged commit c302e43 into master Jul 23, 2026
98 of 100 checks passed
@odow
odow deleted the od/fix-product-of-sets branch July 23, 2026 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

ProductOfSets is wrong when a constraint has zero rows

2 participants