Avoid exception handling for verifying algorithm validity #212
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently, when a `Symbol` is passed to the `Opt` constructor or the `algorithm_name` function, the corresponding `Algorithm` object is obtained via a `Dict` lookup inside of a `try`/`catch`, where the `catch` permits throwing a more informative error. The use of exception handling here isn't necessary though; rather than using `getindex` and catching any resulting exception, we can instead use `get` with a default value that does not correspond to a valid algorithm and check for equality with that. (Alternatively we could guard the `getindex` with a `haskey` but that requires performing the lookup twice.) This change to using `get` is implemented here. To avoid duplicating the code that checks a `Symbol` for validity as an algorithm across multiple functions, I've defined a constructor method `Algorithm(::Symbol)` that includes this check. This also provides a pleasant simplification of the methods required, since `Symbol` and `Integer` inputs can now go through the same `Opt` and `algorithm_name` methods.
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #212 +/- ##
==========================================
+ Coverage 68.46% 68.83% +0.37%
==========================================
Files 2 2
Lines 799 799
==========================================
+ Hits 547 550 +3
+ Misses 252 249 -3 ☔ View full report in Codecov by Sentry. |
|
It's weird, I'm a member of JuliaOpt but I can't actually do anything in the org, including request PR reviews. So manually pinging @stevengj for review here. |
stevengj
approved these changes
Mar 5, 2024
|
LGTM. |
stevengj
reviewed
Mar 5, 2024
Co-authored-by: Steven G. Johnson <[email protected]>
|
FYI I also can't merge PRs here |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, when a
Symbolis passed to theOptconstructor or thealgorithm_namefunction, the correspondingAlgorithmobject is obtained via aDictlookup inside of atry/catch, where thecatchpermits throwing a more informative error. The use of exception handling here isn't necessary though; rather than usinggetindexand catching any resulting exception, we can instead usegetwith a default value that does not correspond to a valid algorithm and check for equality with that. (Alternatively we could guard thegetindexwith ahaskeybut that requires performing the lookup twice.) This change to usinggetis implemented here.To avoid duplicating the code that checks a
Symbolfor validity as an algorithm across multiple functions, I've defined a constructor methodAlgorithm(::Symbol)that includes this check. This also provides a pleasant simplification of the methods required, sinceSymbolandIntegerinputs can now go through the sameOptandalgorithm_namemethods.