-
Notifications
You must be signed in to change notification settings - Fork 109
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
Populate inform dict for all optimizers #394
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #394 +/- ##
==========================================
+ Coverage 73.72% 74.07% +0.34%
==========================================
Files 22 22
Lines 3292 3290 -2
==========================================
+ Hits 2427 2437 +10
+ Misses 865 853 -12 ☔ View full report in Codecov by Sentry. |
The flake8 error is fixed in #392, we need to merge that PR before this one |
@marcomangano I would prefer if we do a patch bump asap rather than waiting for #388 (which I expect will take a bit of time?) As an aside, I do miss my sudo powers... |
#388 is ready to go once this is merged |
sol_inform = {} | ||
# sol_inform['value'] = inform | ||
# sol_inform['text'] = self.informs[inform[0]] | ||
sol_inform = {"value": "", "text": ""} |
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.
Was "value" intentionally set to a string value in these optimizers?
As far as I can tell, this was always meant to be an integer return code, and we have logic that expects that to be the case:
File "/home/swryan/dev/OpenMDAO/openmdao/drivers/pyoptsparse_driver.py", line 680, in run
if exit_status > 2:
^^^^^^^^^^^^^^^
TypeError: '>' not supported between instances of 'str' and 'int'
Just wondering what the circumstances would be where we should expect this value to be a string...
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.
Hmmm.. I see that we were checking if "value" was in the dict.. and if it is there we expect it to be an int....
So we were handling the case for the optimizers that didn't populate it... I guess we should consider an empty string value to be an indication that the optimizer doesn't provide the value...
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.
Yeah I see, I guess we need to agree on a convention for optimizers that don't have any informs. I'm happy to do whatever you guys want, which could be
None
- empty string (current)
- some default inform that just says optimization completed
In the longer term, we should probably make dataclasses and type annotate these better.
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.
In retrospect this PR is probably not the best implementation, and was done purely to make the solution object printing work without changing the logic there...
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.
yeah, it may be that just updating pyOpt_solution.py to only print values for those keys if they exist might a preferable solution, keeping the semantics/typing a bit more rigorous, but it's easy enough for us to handle the empty string (which we will do). Also agree with your long term thoughts.
Purpose
Closes #393.
Expected time until merged
1 day.
Type of change
Testing
Checklist
flake8
andblack
to make sure the Python code adheres to PEP-8 and is consistently formattedfprettify
or C/C++ code withclang-format
as applicable