pyomo.doe adding more verbose output for sensitivity analysis #3525
Conversation
|
@adowling2 @djlaky ready for early feedback |
<
8000
/tr>
|
Next step:
|
|
@smondal13 See #3532 for an example of what @jsiirola is suggesting |
…alue checking in compute_FIM_full_factorial()
…pute_FIM_metrics` function.
… doe object that uses the "reactor_experiment" example in doe directory. Also added the png files to gitignore that are stored when "reactor_example.py" is run.
6e68c2b to
e9e83b6
Compare
|
@smondal13 could you run See: https://pyomo.readthedocs.io/en/latest/contribution_guide.html#coding-standards |
pyomo/contrib/doe/doe.py
Outdated
| fim_factorial_results: a dictionary of the results with the following keys and | ||
| their corresponding values as a list. Each element in the list corresponds to | ||
| a different design point in the full factorial space. | ||
| "log10 D-opt": list of log10(D-optimality) | ||
| "log10 A-opt": list of log10(A-optimality) | ||
| "log10 E-opt": list of log10(E-optimality) | ||
| "log10 ME-opt": list of log10(ME-optimality) | ||
| "eigval_min": list of minimum eigenvalues | ||
| "eigval_max": list of maximum eigenvalues | ||
| "det_FIM": list of determinants | ||
| "trace_FIM": list of traces | ||
| "solve_time": list of solve times |
There was a problem hiding this comment.
This docstring doesn't render nicely. See: https://pyomo--3525.org.readthedocs.build/en/3525/api/pyomo.contrib.doe.doe.DesignOfExperiments.html#pyomo.contrib.doe.doe.DesignOfExperiments.compute_FIM_full_factorial
Try experimenting with the formatting. Maybe if you indent lines 1512-1522 it will preserve the alignment you have in the docstring.
There was a problem hiding this comment.
I have run black, and the file was not changed. I have changed some formatted string manually which black did not catch. Hope this solves this issue
Also, I have reformatted the docstring. I think now it should format nicely.
| # expected results | ||
| det_expected = np.linalg.det(FIM) | ||
| D_opt_expected = np.log10(det_expected) | ||
|
|
||
| trace_expected = np.trace(FIM) | ||
| A_opt_expected = np.log10(trace_expected) | ||
|
|
||
| E_vals_expected, E_vecs_expected = np.linalg.eig(FIM) | ||
| min_eigval = np.min(E_vals_expected.real) | ||
| if min_eigval <= 0: | ||
| E_opt_expected = np.nan | ||
| else: | ||
| E_opt_expected = np.log10(min_eigval) | ||
|
|
||
| cond_expected = np.linalg.cond(FIM) | ||
|
|
||
| ME_opt_expected = np.log10(cond_expected) |
There was a problem hiding this comment.
You typically want to hard-code the expected results in the test instead of copying the function code here. This isn't really testing anything except that the code is the same in both places. If the numpy behavior ever changed resulting in different values, this test would not catch it and would always pass.
There was a problem hiding this comment.
I have now hardcoded the values and the arrays.
|
|
||
|
|
||
| class TestFIMWarning(unittest.TestCase): | ||
| def test_FIM_eigenvalue_warning(self): |
| self.assertEqual(ME_opt, ME_opt_expected) | ||
|
|
||
|
|
||
| class TestFIMWarning(unittest.TestCase): |
There was a problem hiding this comment.
You should include this as another test under the existing class. No need to have separate classes.
| class TestFIMWarning(unittest.TestCase): |
pyomo/contrib/doe/doe.py
Outdated
There was a problem hiding this comment.
Why did you add this as a staticmethod instead of a utility function outside of the class?
There was a problem hiding this comment.
@blnicho Moving this to utils.py is a really good suggestion
There was a problem hiding this comment.
All the standalone functions are moved to utils.py
pyomo/contrib/doe/doe.py
Outdated
| # loggers for the functions | ||
| function_logger = logging.getLogger(__name__) | ||
| function_logger.setLevel(logging.WARNING) |
There was a problem hiding this comment.
The logger for this module is retrieved above, with the logging level set based on the user input. See:
pyomo/pyomo/contrib/doe/doe.py
Lines 218 to 220 in 9efc82d
I'm worried that grabbing the logger for this module again and setting the level here will lead to weird edge cases with the logging code in the class. What's the reasoning for manually setting the level here?
@adowling2 would you be opposed to moving these stand-alone functions into pyomo.contrib.doe.utils?
There was a problem hiding this comment.
If you move these functions into pyomo.contrib.doe.utils, then I would also suggest combining the test files in this PR into a single test_utils.py file.
There was a problem hiding this comment.
Moved the standalone functions to utils.py and compiled all the test files into test_utils.py
…the docstring of the compute_FIM_full factorial.
…ics() to utils.py file.
- All six tests ran successfully. - Deleted the unnecessary files
…. Also, tested get_FIM_metrics() function and it works fine.
|
@blnicho I have implemented your suggestions. It is ready for review now. |
… adding_eigen_values
…reactor_example.py file to have user defined computation. Changed the docstring formatting in utils.py
… version looks fine. Also, changed the import of run_reactor_doe in test_doe_solve.py
There was a problem hiding this comment.
My memory (which may be incorrect) is that @smondal13 ran black without the correct inputs and it changed the string formatting from single quote to double quote. I see this change around line 106 in test_doe_solve.py and a few other places.
It only touches a handful of files in Pyomo.DoE so I think we decided it's fine to leave it. Just make sure to use the |
| @@ -11,6 +11,7 @@ | |||
| import json | |||
| import logging | |||
| import os.path | |||
| import idaes.core.solvers.get_solver | |||
There was a problem hiding this comment.
What is this import being used for? We generally want to avoid importing anything from IDAES in Pyomo since it creates a circular dependency.
Fixes # .
Summary/Motivation:
get_labeled_model().reactor_example.pywill help with the testing.Changes proposed in this PR:
compute_FIM_full_factorial()check_FIM()in utils.py by separating it fromcheck_model_FIM()in doe.pycompute_FIM_metrics()in utils.pyget_FIM_metrics()in utils.pyreactor_example.pyLegal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: