Skip to content

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

Draft
odow wants to merge 8 commits into
masterfrom
od/fix-product-of-sets
Draft

[Utilities] fix ProductOfSets to handle zero-dimensional rows#3022
odow wants to merge 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

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