-
Notifications
You must be signed in to change notification settings - Fork 546
Extended Parmest Capability for weighted SSE objective #3535
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
base: main
Are you sure you want to change the base?
Conversation
@slilonfe5 Here is some quick feedback
Feedback on the
|
@adowling2 @djlaky I also updated the calculation for the normal SSE such that we can use the user-supplied measurement error if defined; otherwise, we calculate the measurement error as usual. |
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.
Nice progress. I think it is time to start writing tests for the new capabilities.
@slilonfe5 Once you have the tests ready, tag us for feedback. Also, I think you can skip adding this to the depreciated class. |
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.
Here is more feedback as you work on getting this ready for the Pyomo team to review.
@adowling2 @djlaky I have created a separate method ( I tested these with three examples (2 steady state and 1 dynamic), and all work well. I'm yet to write the test file for these. |
Co-authored-by: Miranda Mundt <[email protected]>
Co-authored-by: Miranda Mundt <[email protected]>
@adowling2 @djlaky @mrmundt @blnicho @jsiirola However, I encountered an issue with the test for other functions, such as bootstrap and likelihood ratio. I plan to discuss it today at the Pyomo Dev meeting. Thanks. |
@adowling2 @djlaky @mrmundt @blnicho @jsiirola Like I mentioned yesterday, this PR is ready for final review. All the tests have passed. Also, based on yesterday's meeting, I will create a new PR to fix the bugs in the current parmest.py file and the examples that fail when running the test_examples.py and test_scenariocreator.py files. The new PR will have to go through before this PR goes through. |
After creating the new PR, please post a link here. |
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 seems like a lot of comments, but there are a few repeats! This is looking pretty good so far; most of my questions/suggestions aren't "necessary" but will make the code cleaner.
try: | ||
solver = pyo.SolverFactory(solver) | ||
res = solver.solve(model, tee=tee) | ||
pyo.assert_optimal_termination(res) | ||
except Exception as e: | ||
raise RuntimeError( | ||
f"Model from experiment did not solve appropriately. Make sure the " | ||
f"model is well-posed. The original error was {e}." | ||
) |
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 a bit redundant. pyo.assert_optimal_termination
will already raise a RuntimeError
; why bother with raising a different one?
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.
@mrmundt The initial plan for raising a different one was to make the error message more clear. Do you suggest having only the code inside the try block (i.e., not do this in a try-except block)?
try: | ||
res = solver.solve(model, tee=tee) | ||
pyo.assert_optimal_termination(res) | ||
except Exception as e: | ||
raise RuntimeError( | ||
f"Model from experiment did not solve appropriately. Make sure the " | ||
f"model is well-posed. The original error was {e}." | ||
) |
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 a bit redundant. pyo.assert_optimal_termination
will already raise a RuntimeError
; why bother with raising a different one?
try: | ||
res = solver.solve(model, tee=tee) | ||
pyo.assert_optimal_termination(res) | ||
except Exception as e: | ||
raise RuntimeError( | ||
f"Model from experiment did not solve appropriately. Make sure the " | ||
f"model is well-posed. The original error was {e}." | ||
) |
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 a bit redundant. pyo.assert_optimal_termination
will already raise a RuntimeError
; why bother with raising a different one?
@mrmundt Thank you for your feedback. I will work on them to make the code cleaner. |
Fixes # .
Summary/Motivation:
Currently, the Parmest SSE objective does not support measurements in different units. This work adds a new capability (i.e., weighted SSE) to Parmest to handle measurements in different units.
Changes proposed in this PR:
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: