Skip to content

Conversation

odow
Copy link
Member

@odow odow commented Aug 14, 2021

A look at what this would look like. I still need to benchmark to cost of always trying to copy names.

Closes #1533

@odow odow added breaking Submodule: Utilities About the Utilities submodule labels Aug 14, 2021
@odow odow added this to the v0.10 milestone Aug 14, 2021
@odow odow changed the title WIP: remove copy_names argument Remove copy_names argument Aug 19, 2021
@odow odow requested a review from blegat August 19, 2021 05:22
@odow
Copy link
Member Author

odow commented Aug 19, 2021

Not sure if/how to benchmark this. It's going to depend on the solver. In most cases, this should be minimal. If it is, it needs to be fixed in the solver, or an option in the solver provided to skip supporting names.

@blegat
Copy link
Member

blegat commented Aug 20, 2021

If it turns out to be noticeable in performance, we can add a filter layer and an option to JuMP/CachingOptimizer to apply this layer to filter out the names. The question is : what would be the default ?
If the default would be to copy names then making this change later is non-breaking.
It seems to make sense to me that names would be copied by default. We can have a performance tips section in JuMP that recommends things like "unset the copy of names, use direct mode, ...". We could even have an option to not even set the names to the moi_backend from JuMP but these will not be the default and we will wait to have a confincing benchmark first and can be done post v1.0 as non-breaking.

@odow
Copy link
Member Author

odow commented Aug 20, 2021

Yeah there are multiple ways to improve performance later, whether that's adding a new layer, adding an option to JuMP, or adding an option to the solver. Either way, I don't think a copy_names argument is the answer.

@blegat
Copy link
Member

blegat commented Aug 20, 2021

Yes, we should aim at having a copy_to with only 2 arguments. Otherwise we are pushing additional complexity to all the solver wrappers.

@odow odow merged commit 43b6c92 into master Aug 21, 2021
@odow odow deleted the od/copyto branch August 21, 2021 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Submodule: Utilities About the Utilities submodule
Development

Successfully merging this pull request may close these issues.

The need for copy_names
2 participants