-
Notifications
You must be signed in to change notification settings - Fork 517
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
Update PyROS Solver Argument Resolution and Validation Routines #3126
Update PyROS Solver Argument Resolution and Validation Routines #3126
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3126 +/- ##
==========================================
+ Coverage 88.33% 88.35% +0.02%
==========================================
Files 833 834 +1
Lines 92648 92648
==========================================
+ Hits 81836 81862 +26
+ Misses 10812 10786 -26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple comments after a quick read-through
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine to merge. Longer term, I'd like kto revisit if we need as many functors as we currently have.
default_pyros_solver_logger = setup_pyros_logger() | ||
|
||
|
||
class LoggerType: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this (and PositiveIntOrMinusOne
below) a functor and not just a plain function? Is it just so that you can define the domain_name()
method? If that is the case, then I think we should update ConfigValue to allow for domain_name
to just be an attribute - then these can be simple functions and not need to be full classes / instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to define the domain_name()
method. I agree that updating ConfigValue
to make domain_name
an attribute would help simplify.
Fixes #2428
Fixes #2437
Changes proposed in this PR:
common.config
more extensively for argument-wise validation.ConfigDict
#2754 in lieu of that of Update PyROS Solver and Webpage Documentation #2707 to generate the docstring ofPyROS.solve()
. Remove interface of Update PyROS Solver and Webpage Documentation #2707.Legal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: