Skip to content

VectorAffineFunction easy constructor #2626

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

Closed
wants to merge 8 commits into from

Conversation

matbesancon
Copy link
Contributor

VectorAffineFunction has always been a massive pain to construct.
This added constructor builds one from a vector of scalar affine functions, which is the mental model most people will have from a VAF.

This can drastically simplify a lot of code

@odow
Copy link
Member

odow commented Feb 14, 2025

I'm in favor of this. It hurts me every time I try to create a VAF.

Thoughts on VectorQuadraticFunction as well?

@matbesancon
Copy link
Contributor Author

Thoughts on VectorQuadraticFunction as well?

I'll take a shot in the next days, it will be in a separate PR

@odow
Copy link
Member

odow commented Feb 14, 2025

I can do it

@blegat
Copy link
Member

blegat commented Feb 15, 2025

I'm not sure we need this since we already have MOI.Utilities.vectorize

@matbesancon
Copy link
Contributor Author

the constructor is easier to discover and search for, it has zero cost, gives you a way to construct the VAF that is close to the way it's printed, and the mental model makes sense -> constructing a vector affine expression takes a vector of scalar affine expressions.

MOI.Utilities.vectorize or MOI.Utilities.operate are both awkward to use constructors in disguise IMO

@blegat
Copy link
Member

blegat commented Feb 15, 2025

We could consider having two names for the same thing but we shouldn't duplicate the implementation. One of the two should call the other one

@odow
Copy link
Member

odow commented Feb 15, 2025

We could document vectorize in the default constructor?

But case in point: I forget about vectorize

@blegat
Copy link
Member

blegat commented Feb 15, 2025

Yes, and vectorize should call the constructor instead of duplicating the code

@joaquimg
Copy link
Member

joaquimg commented Feb 16, 2025

I like this PR, I almost wrote it twice recently.
Can we just use vectorize inside this new function?

@odow
Copy link
Member

odow commented Feb 16, 2025

Nah we should go the other way around: vectorize use this constructor. I can update the PR.

@blegat
Copy link
Member

blegat commented Feb 17, 2025

Now the code is a bit weird. vectorize was sharing code between creating VQF and VAF. I would expect the code to also be faster as it preallocate the right dimension. I'd move that code to src/function for both the VQF and VAF constructor and then we can keep vectorize in MOI.Utilities that just calls the constructor.

@joaquimg
Copy link
Member

A more naive constructor is not terrible for tests as functions would be small. But vectorize can be used in very large expressions in production code, so it should be preallocating.

@odow
Copy link
Member

odow commented Feb 17, 2025

Closing in favor of #2636

@odow odow closed this Feb 17, 2025
@matbesancon matbesancon deleted the vaf-constructor branch February 18, 2025 07: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.

4 participants