pyomo icon indicating copy to clipboard operation
pyomo copied to clipboard

Add Pyomo.DOE to contrib

Open jialuw96 opened this issue 3 years ago • 15 comments

Fixes # .

Summary/Motivation: Contribute model-based design of experiments framework to Pyomo

Tasks to complete before merging:

  • [ ] Merge PR on fork https://github.com/jialuw96/pyomo/pull/5
  • [x] Add documentation in sphinx
  • [ ] Confirm tests work
  • [x] Add CCSI2 licence

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:

  1. I agree my contributions are submitted under the BSD license.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

jialuw96 avatar Feb 21 '22 18:02 jialuw96

@jialuw96 @adowling2 what's the status on this WIP PR? Are you waiting on input from us?

As a general rule we tend to not focus any of our PR review resources on WIP PRs unless specifically asked.

blnicho avatar Mar 30 '22 06:03 blnicho

Marked as ready for review to start working on finalizing the tests. @jsiirola @kaklise @adowling2 This is ready for first round of comments.

jialuw96 avatar Apr 08 '22 18:04 jialuw96

@jialuw96 I get an error when running the tutorial notebook. Cell 7 when calling compute_FIM, "NameError: name 'if_Cholesky' is not defined"

kaklise avatar Apr 19 '22 15:04 kaklise

@jialuw96 I get an error when running the tutorial notebook. Cell 7 when calling compute_FIM, "NameError: name 'if_Cholesky' is not defined"

Error solved. Now the tutorial notebook should be able to run without bug.

jialuw96 avatar Apr 19 '22 17:04 jialuw96

Tests are failing because idaes is being imported unconditionally in test_reactor_example.py. @jialuw96 last week you mentioned that you were importing idaes in your testing example in order to get Ipopt. You should avoid importing idaes from files in the Pyomo repo because it could lead to issues with circular imports. You should assume the user would have Ipopt in their search path without needing to import idaes.

Also, Pyomo does not have a strict dependency on Ipopt. This means that your testing examples using Ipopt should 1) verify that Ipopt is available and 2) skip the test when Ipopt is not available. You can find an example of this here: https://github.com/Pyomo/pyomo/blob/main/pyomo/dae/tests/test_initialization.py (see lines 26 and 102)

blnicho avatar May 03 '22 18:05 blnicho

@blnicho Hi Bethany, we now have some errors from the code in docs. It seems that the code runs and print some sentences, and results, which is not expected by the doc. These sentences and results are printed by Pyomo.DOE by default. I attach two examples here. Should I prevent the code to print any sentences and not solve the problem in doctest? Thank you!

Here is an example where the example code prints sentences:


File "contributed_packages/doe/doe.rst", line 312, in default Failed example: measure_class = doe.Measurements(measure_pass, variance=measure_variance) # Use Pyomo.DoE.Measurements to achieve a measurement object Expected nothing Got: All measurements are flattened. Flatten measurement name: ['C_index_CA', 'C_index_CB', 'C_index_CC']

Here is an example where the example code solves problem and shows output:


File "contributed_packages/doe/doe.rst", line 336, in default Failed example: result = doe_object.compute_FIM(exp1,mode=sensi_opt, FIM_store_name = 'dynamic.csv', store_output = 'store_output') Expected nothing Got: This scenario: {'A1': {0: 84.87478999999999}, 'A2': {0: 371.72}, 'E1': {0: 7.78}, 'E2': {0: 15.05}, 'jac-index': {'A1': [0], 'A2': [0], 'E1': [0], 'E2': [0]}, 'eps-abs': {'A1': 0.08487478999999999, 'A2': 0.37172000000000005, 'E1': 0.0077800000000000005, 'E2': 0.015050000000000001}, 'scena-name': [0]} Ipopt 3.13.2: linear_solver=ma57 halt_on_ampl_error=yes max_iter=3000 <BLANKLINE> <BLANKLINE> ****************************************************************************** This program contains Ipopt, a library for large-scale nonlinear optimization. Ipopt is released as open source code under the Eclipse Public License (EPL). For more information visit http://projects.coin-or.org/Ipopt <BLANKLINE> This version of Ipopt was compiled from source code available at https://github.com/IDAES/Ipopt as part of the Institute for the De Advanced Energy Systems Process Systems Engineering Framework (IDAES PSE Framework) Copyright (c) 2018-2019. See https://github.com/IDAES/idaes-pse. <BLANKLINE> This version of Ipopt was compiled using HSL, a collection of Fortran codes for large-scale scientific computation. All technical papers, sales and publicity material resulting from use of the HSL codes within IPOPT must contain the following acknowledgement: HSL, a collection of Fortran codes for large-scale scientific computation. See http://www.hsl.rl.ac.uk./ ****************************************************************************** <BLANKLINE> This is Ipopt version 3.13.2, running with linear solver ma57. <BLANKLINE> Number of nonzeros in equality constraint Jacobian...: 2955

(and then there's the IPOPT output)

jialuw96 avatar Jun 07 '22 18:06 jialuw96

@jialuw96 you can run the documentation tests locally by going into the pyomo/doc/OnlineDocs directory and running the command make doctest -d

For examples of writing doctests that include output take a look at these rst files: https://github.com/Pyomo/pyomo/blob/main/doc/OnlineDocs/contributed_packages/trustregion.rst https://github.com/Pyomo/pyomo/blob/main/doc/OnlineDocs/contributed_packages/sensitivity_toolbox.rst

In particular, if your code snippets produce output (like the print statements at the end of your doe.rst file) then you need to include that output in the code snippet. The doctest will verify that the code runs and the same output is produced. If an exact comparison doesn't make sense or is too fragile (i.e. full Ipopt log or floating point number with many decimal places) then there are additional options you can specify in the doctest (... or :skipif: or doctest: +SKIP) or you can be more careful in what is printed (rounding floats to fixed number of decimal places).

blnicho avatar Jun 14 '22 23:06 blnicho

@blnicho Hi Bethany, I included the output of print sentences, and skip commands that would generate IPOPT outputs. The error now is a MPI error in linux/3.8/mpi as:

Run # Manually invoke the DAT parser so that parse_table_datacmds.py /home/runner/work/_temp/56e3fe61-c523-4458-8cf3-31fce554ad5f.sh: line 5: mpirun: command not found Error: Process completed with exit code 127.

and codev error in proces-coverage:

[2022-06-21T17:13:47.903Z] ['error'] There was an error running the uploader: Error uploading to https://codecov.io: Error: There was an error fetching the storage URL during POST: 404 - {'detail': ErrorDetail(string='Could not find a repository, try using repo upload token', code='not_found')} Error: Codecov: Failed to properly upload: The process '/home/runner/work/_actions/codecov/codecov-action/v2/dist/codecov' failed with exit code 255

Could you point out how to debug these errors from my side? Thank you!

jialuw96 avatar Jun 21 '22 17:06 jialuw96

Kanishka is also having codecov issues. Perhaps discuss this at the Pyomo dev call today?

adowling2 avatar Jun 21 '22 17:06 adowling2

Codecov Report

Base: 87.04% // Head: 86.68% // Decreases project coverage by -0.37% :warning:

Coverage data is based on head (4d504f1) compared to base (09e204c). Patch coverage: 66.05% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2294      +/-   ##
==========================================
- Coverage   87.04%   86.68%   -0.37%     
==========================================
  Files         757      766       +9     
  Lines       84596    86224    +1628     
==========================================
+ Hits        73638    74742    +1104     
- Misses      10958    11482     +524     
Flag Coverage Δ
linux 84.15% <66.05%> (-0.30%) :arrow_down:
osx 73.59% <17.41%> (-0.92%) :arrow_down:
other 84.34% <66.05%> (-0.30%) :arrow_down:
win 81.54% <66.05%> (-0.26%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pyomo/contrib/doe/result.py 34.27% <34.27%> (ø)
pyomo/contrib/sensitivity_toolbox/sens.py 97.40% <50.00%> (-0.31%) :arrow_down:
pyomo/contrib/doe/doe.py 67.28% <67.28%> (ø)
pyomo/contrib/doe/measurements.py 81.98% <81.98%> (ø)
pyomo/contrib/doe/scenario.py 90.19% <90.19%> (ø)
pyomo/contrib/doe/example/reactor_grid_search.py 93.47% <93.47%> (ø)
pyomo/contrib/doe/example/reactor_compute_FIM.py 94.44% <94.44%> (ø)
pyomo/contrib/doe/example/reactor_optimize_doe.py 96.15% <96.15%> (ø)
pyomo/contrib/doe/example/reactor_kinetics.py 97.93% <97.93%> (ø)
pyomo/contrib/doe/__init__.py 100.00% <100.00%> (ø)
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Jul 05 '22 21:07 codecov[bot]

@blnicho Hi Bethany, the comments on doe.rst are addressed now and unused figures are removed. Thank you!

jialuw96 avatar Jul 18 '22 19:07 jialuw96

@blnicho The comments are addressed. Currently no other questions. Thank you!

jialuw96 avatar Aug 02 '22 17:08 jialuw96

@jialuw96 I just pushed a minor change to your notebook example. This just removes the cell ids and tells Jupyter to use a slightly older notebook format.

blnicho avatar Aug 24 '22 19:08 blnicho

Hi Bethany, thank you for comments! We are still working on them. I would like to follow up about changing the tutorial notebook into a test:

  1. What is preferred is we make the examples a .py file, create a .py file for each function I call Pyomo.DOE, and add a main function to each .py file. In the test folder, I then add a .py file where I call every .py file for this notebook as a test. Right?
  2. We can still keep the tutorial notebook, right?

Thank you!

jialuw96 avatar Sep 13 '22 18:09 jialuw96

@blnicho Hi Bethany, sorry I cannot attend the meeting today due to a meeting conflict. I have modified this version and it passes all tests in my local machine; However it shows one error message I never saw in the workflow, it is pasted below:

/Users/runner/work/pyomo/pyomo/pyomo/environ/tests/test_package_layout.py:72: AssertionError ------ generated xml file: /Users/runner/work/pyomo/pyomo/TEST-pyomo.xml ------- =========================== short test summary info ============================ FAILED pyomo/core/tests/unit/test_symbolic.py::SymbolicDerivatives::test_trig_fuctions FAILED pyomo/environ/tests/test_package_layout.py::TestPackageLayout::test_for_init_files ===== 2 failed, 9690 passed, 5016 skipped, 11 xfailed in 900.13s (0:15:00) ===== Error: Process completed with exit code 1.

Do you have idea how to fix this? Thank you!

Jialu

jialuw96 avatar Sep 20 '22 18:09 jialuw96

@jialuw96 I'm not seeing the same test failures you reported. Looks like most of the failing jobs are all failing because Ipopt is failing to converge. This is the test that needs to be fixed:

pyomo/contrib/doe/tests/test_example.py::TestReactorExample::test_reactor_optimize_doe

The PyPy job also revealed a couple tests that require optional dependencies (pandas or scipy):

FAILED pyomo/contrib/doe/tests/test_example.py::TestReactorExample::test_reactor_compute_FIM
FAILED pyomo/contrib/doe/tests/test_example.py::TestReactorExample::test_reactor_grid_search

You'll need to check if these dependencies are available and skip the tests when they aren't installed (similar to how you're checking ipopt_available).

blnicho avatar Oct 21 '22 22:10 blnicho

@blnicho Hi Bethany, I think all tests get passed now. I would like to confirm on what are needed for the 3rd round of reviews:

  • [x] Keep copyright info of Pyomo and CCSI2 in every file, while moving CCSI2 license to be a separate .txt file.
  • [x] Use logging instead of print statements
  • [x] Remove build directory

Is there anything else that you are expecting? I plan to finish these and make this repo ready for next round of review this and the next week. Thank you!

jialuw96 avatar Nov 04 '22 02:11 jialuw96

@jialuw96 I added a couple more items to the checklist in your last comment.

blnicho avatar Nov 04 '22 16:11 blnicho

@adowling2 @blnicho @jsiirola This code is ready for the next round of reviews. Thank you!

jialuw96 avatar Nov 08 '22 17:11 jialuw96

@jialuw96 As we work through these comments, we need to document any enchancements we decide to delay to a future PR on the new meta issue #2610 .

adowling2 avatar Nov 17 '22 21:11 adowling2

@blnicho @jsiirola Just to confirm, our intention is to merge this after the November release. We then have a few smaller PRs planned for January and early February (see #2610) to get merged before the next Pyomo release.

adowling2 avatar Nov 24 '22 14:11 adowling2

@blnicho @jsiirola @adowling2 This pull request is ready for review. Thank you!

jialuw96 avatar Dec 13 '22 19:12 jialuw96

@blnicho @jsiirola @adowling2 This PR is ready for review. Thank you!

jialuw96 avatar Jan 10 '23 22:01 jialuw96

@jialuw96 You replied to my comments but I think you forgot to push the updates.

adowling2 avatar Jan 11 '23 22:01 adowling2

@kaklise @blnicho @jsiirola I think this is ready for one of you to complete the 2nd review.

adowling2 avatar Jan 12 '23 16:01 adowling2

@blnicho This PR is ready for review. Thank you!

jialuw96 avatar Jan 23 '23 21:01 jialuw96

@jialuw96 I'm still seeing errors in the ReadtheDocs build with some of the autoclass commands. This is the output:

WARNING: autodoc: failed to import class 'fim_doe.DesignOfExperiments' from module 'pyomo.contrib.doe'; the following exception was raised:
No module named 'pyomo.contrib.doe.fim_doe'
/home/docs/checkouts/readthedocs.org/user_builds/pyomo/checkouts/2294/pyomo/contrib/doe/measurements.py:docstring of pyomo.contrib.doe.measurements.Measurements.__init__:4: WARNING: Definition list ends without a blank line; unexpected unindent.
WARNING: autodoc: failed to import class 'FIM_result' from module 'pyomo.contrib.doe'; the following exception was raised:
Traceback (most recent call last):
  File "/home/docs/checkouts/readthedocs.org/user_builds/pyomo/envs/2294/lib/python3.7/site-packages/sphinx/util/inspect.py", line 376, in safe_getattr
    return getattr(obj, name, *defargs)
AttributeError: module 'pyomo.contrib.doe' has no attribute 'FIM_result'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/docs/checkouts/readthedocs.org/user_builds/pyomo/envs/2294/lib/python3.7/site-packages/sphinx/ext/autodoc/importer.py", line 98, in import_object
    obj = attrgetter(obj, mangled_name)
  File "/home/docs/checkouts/readthedocs.org/user_builds/pyomo/envs/2294/lib/python3.7/site-packages/sphinx/ext/autodoc/__init__.py", line 306, in get_attr
    return autodoc_attrgetter(self.env.app, obj, name, *defargs)
  File "/home/docs/checkouts/readthedocs.org/user_builds/pyomo/envs/2294/lib/python3.7/site-packages/sphinx/ext/autodoc/__init__.py", line 2804, in autodoc_attrgetter
    return safe_getattr(obj, name, *defargs)
  File "/home/docs/checkouts/readthedocs.org/user_builds/pyomo/envs/2294/lib/python3.7/site-packages/sphinx/util/inspect.py", line 392, in safe_getattr
    raise AttributeError(name) from exc
AttributeError: FIM_result

WARNING: autodoc: failed to import class 'Grid_Search_Result' from module 'pyomo.contrib.doe'; the following exception was raised:
Traceback (most recent call last):
  File "/home/docs/checkouts/readthedocs.org/user_builds/pyomo/envs/2294/lib/python3.7/site-packages/sphinx/util/inspect.py", line 376, in safe_getattr
    return getattr(obj, name, *defargs)
AttributeError: module 'pyomo.contrib.doe' has no attribute 'Grid_Search_Result'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/docs/checkouts/readthedocs.org/user_builds/pyomo/envs/2294/lib/python3.7/site-packages/sphinx/ext/autodoc/importer.py", line 98, in import_object
    obj = attrgetter(obj, mangled_name)
  File "/home/docs/checkouts/readthedocs.org/user_builds/pyomo/envs/2294/lib/python3.7/site-packages/sphinx/ext/autodoc/__init__.py", line 306, in get_attr
    return autodoc_attrgetter(self.env.app, obj, name, *defargs)
  File "/home/docs/checkouts/readthedocs.org/user_builds/pyomo/envs/2294/lib/python3.7/site-packages/sphinx/ext/autodoc/__init__.py", line 2804, in autodoc_attrgetter
    return safe_getattr(obj, name, *defargs)
  File "/home/docs/checkouts/readthedocs.org/user_builds/pyomo/envs/2294/lib/python3.7/site-packages/sphinx/util/inspect.py", line 392, in safe_getattr
    raise AttributeError(name) from exc
AttributeError: Grid_Search_Result

blnicho avatar Jan 25 '23 16:01 blnicho