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

MatrixOfConstraints and IndexMap #1772

Open
blegat opened this issue Mar 3, 2022 · 6 comments
Open

MatrixOfConstraints and IndexMap #1772

blegat opened this issue Mar 3, 2022 · 6 comments
Labels
Submodule: Utilities About the Utilities submodule
Milestone

Comments

@blegat
Copy link
Member

blegat commented Mar 3, 2022

Document that solvers can rely on the fact that MatrixOfConstraints does not implement deletion of rows and columns and hence, the OptimizerCache can be done in a way such that the index map is identity.

When using OrderedProductOfSets, the solver will need to keep the product of set as a field after copy_to to handle the mapping of indices to ranges, such mapping is needed for conic solvers anyway.

For linear solvers, there is also the choice of using MixOfScalarSets for which the mapping is trivial so no need to keep the product of set as a field.

Since that would make many solvers use identity_index_map, it gives incentive to make this function cheaper. One option is to make CleverDict even more clever with a bool is_identity and only start using vector when the mapping starts being non-trivial. That would also benefit to solvers not using identity_index_map but for which the mapping is trivial most of the time.

See jump-dev/Cbc.jl#189 (comment)

@odow odow added the Submodule: Utilities About the Utilities submodule label Mar 3, 2022
@odow odow added this to the v1.x milestone Mar 3, 2022
@odow
Copy link
Member

odow commented Oct 23, 2023

Is there any benchmark we could run that would show this is meaningful?

No one has complained about it, and I don't recall it ever appearing in our profiling, so I'm hesitant to add yet more complicated machinery to the index maps.

@blegat
Copy link
Member Author

blegat commented Oct 23, 2023

IIRC, you were mentioning a use case for the identity index map recently but I don't recall. Anyway, this comes from the benchmarking of SCS that we did for the MOI paper (which started the development of MatrixOfConstraints). Most of the time remaining was building the index maps. Now, for conic solvers, this is still negligible compared to the solve time so you won't see complaints, it's mostly when benchmarking to model creation time. For LP's, it might be more noticeable but LP solvers don't use MatrixOfConstraints for now (maybe they could ? :) )

@odow
Copy link
Member

odow commented Oct 23, 2023

Now, for conic solvers, this is still negligible compared to the solve time so you won't see complaints, it's mostly when benchmarking to model creation time

This seems like a low priority then. I'm very hesitant to add more complexity to MOI to chase performance wins.

@blegat
Copy link
Member Author

blegat commented Oct 24, 2023

Yes, it's not high priority

@odow
Copy link
Member

odow commented Apr 10, 2024

I'm very hesitant to add more complexity to MOI to chase performance wins.

I still believe this, and I'm tempted to close this issue because of it. I don't remember the last time that this was an issue in any benchmark or post on Discourse.

@blegat
Copy link
Member Author

blegat commented Apr 10, 2024

We've been discussing the overhead of IndexMap in a few other issues as well, I still think it is worth it, not only for MatrixOfConstraints

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Submodule: Utilities About the Utilities submodule
Development

No branches or pull requests

2 participants