Skip to content

Conversation

odow
Copy link
Member

@odow odow commented Aug 8, 2021

Closes #1503

@odow odow added the breaking label Aug 8, 2021
@joaquimg
Copy link
Member

joaquimg commented Aug 9, 2021

docs could be a bit more direct:

... The function
[`MathOptInterface.supports_incremental_interface`](@ref) can be used to check
whether `dest` supports the copying a model incrementally.
... The function
[`MathOptInterface.supports_incremental_interface`](@ref) can be used to check
whether this function will work since it will check wether `dest` supports the copying of a model incrementally.

:default_copy_to,
)
return default_copy_to(dest, src, true)
end
Copy link
Member Author

Choose a reason for hiding this comment

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

It seems crazy, but this has been sitting here since MOI 0.2.0! 1ae8661

@odow odow requested a review from blegat August 9, 2021 06:30
filter_constraints::Union{Nothing,Function} = nothing,
)
if !MOI.supports_incremental_interface(dest, copy_names)
error(
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering whether it makes sense to remove this error.
MOI.supports_incremental_interface is what is used by instantiate.
Would there be a case where you want instantiate to add a cache but you would still like to be able to call default_copy_to?
Removing the error is nonbreaking so it might be fine for now

Copy link
Member Author

Choose a reason for hiding this comment

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

This check is trivial and will be in-lined. It also prevents a later error like "add variable not allowed" in some cases. I'd suggest we leave it in, at least until all the solvers are updated. We could remove it for 1.0 if it's not needed.

Copy link
Member

Choose a reason for hiding this comment

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

This check is trivial and will be in-lined

Yes, I was not worried about peformance

I'd suggest we leave it in, at least until all the solvers are updated. We could remove it for 1.0 if it's not needed.

Sounds good

@odow odow merged commit 34cfd17 into master Aug 10, 2021
@odow odow deleted the od/automatic-copy-to branch August 10, 2021 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

Default implementation for copy

3 participants