Skip to content

Conversation

odow
Copy link
Member

@odow odow commented Sep 20, 2023

This one deserved a separate commit because the changes are a little subtle.

The various Bridge submodules define EmptyMap as a fallback for when an AbstractBridgeOptimizer doesn't have an associated set of arcs (like a Variable.SingleBridgeOptimizer). But this makes all calls of .bridges(b) type unstable. And sometimes we really do want either, but sometimes (like the ones annnotated here), we want only the explicit Map.

For MOI v2, we could consider refactoring this code to remove EmptyMap in favor of a more explicit fallback like nothing.

This PR brings us down to ═════ 42 possible errors found ═════

This one deserved a separate commit because the changes are a little
subtle.

The various Bridge submodules define EmptyMap as a fallback for when
an AbstractBridgeOptimizer doesn't have an associated set of arcs
(like a Variable.SingleBridgeOptimizer). But this makes all calls
of .bridges(b) type unstable. And sometimes we really do want either,
but sometimes (like the ones annnotated here), we want only the explicit
Map.

For MOI v2, we could consider refactoring this code to remove EmptyMap
in favor of a more explicit fallback like nothing.
@odow odow requested a review from blegat September 20, 2023 05:01
Copy link
Member

@blegat blegat left a comment

Choose a reason for hiding this comment

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

Not sure why nothing would be more convenient than EmptyMap but the changes of this PR make sense. I'm starting to like JET more and more

@odow
Copy link
Member Author

odow commented Sep 20, 2023

Not sure why nothing would be more convenient than EmptyMap

I think it'd just make it clearer what's actually going on. We don't really want to use EmptyMap for anything other than to signal that there isn't a map.

@blegat
Copy link
Member

blegat commented Sep 20, 2023

We don't really want to use EmptyMap for anything other than to signal that there isn't a map.

IIRC, it was helpful because it implements the Map API so instead of doing if isnothing all over the place in bridge_optimizer.jl, you can just call the API and it will work either way.

@odow
Copy link
Member Author

odow commented Sep 20, 2023

it was helpful because it implements the Map API so instead of doing if isnothing all over the place in bridge_optimizer.jl, you can just call the API and it will work either way.

Only in some places though. It doesn't implement any of these call sites which need annotations (because they wouldn't get called for a Variable.SingleBridgeOptimizer).

@blegat
Copy link
Member

blegat commented Sep 20, 2023

It's possible, I'd have to take another look, it's been a long time

@odow odow merged commit b605464 into master Sep 20, 2023
@odow odow deleted the od/jet5 branch September 20, 2023 09:36
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.

2 participants