[Pyomo.DoE] update documentation#3812
Conversation
…sted by running it.
|
@smondal13 - We have standardized to "Parmest" or "parmest", not "ParmEst". It's the source of the typo checker yelling at you. |
|
Thank you @mrmundt |
|
@adowling2 @blnicho This PR is ready for review Note: Merge PR #3803 before this one. The documentation in this PR contains updated information from that PR |
| :end-before: End sensitivity analysis | ||
|
|
||
| An example output of the code above, a design exploration for the initial concentration and temperature as experimental design variables with 9 values, produces the four figures summarized below: | ||
| .. code-block:: python |
There was a problem hiding this comment.
Why are you embedding the example instead of using .. literalinclude::? As implemented, this example is no longer being tested (which is a problem).
There was a problem hiding this comment.
I copied and pasted the code from the example to skip the booleans that we have in the original example. However, I agree that testing this is important. So, I have added .. literalinclude:: and removed the copy-pasted code.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3812 +/- ##
==========================================
+ Coverage 89.67% 89.68% +0.01%
==========================================
Files 908 908
Lines 106735 106845 +110
==========================================
+ Hits 95717 95827 +110
Misses 11018 11018
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
smondal13
changed the title
adowling2
left a comment
adowling2
left a comment
There was a problem hiding this comment.
Please update the list of contributors.
Should we also add the figures that explain the experiment abstraction? Or should we instead link to our workshop material? https://dowlinglab.github.io/pyomo-doe/Readme.html
| **Pyomo.DoE** (Pyomo Design of Experiments) is a Python library for model-based design of experiments using science-based models. | ||
|
|
||
| Pyomo.DoE was developed by **Jialu Wang** and **Alexander W. Dowling** at the University of Notre Dame as part of the `Carbon Capture Simulation for Industry Impact (CCSI2) <https://github.com/CCSI-Toolset/>`_. | ||
| Pyomo.DoE was developed by **Jialu Wang** and **Alexander W. Dowling** at the |
There was a problem hiding this comment.
Let's expand this text to say "Pyomo.DoE was originally created by ... as part of the ... Significant improvements and extensions were contributed by (list people) with funding from (list and link to PrOMMiS) and the University of Notre Dame.
There was a problem hiding this comment.
In my opinion, adding a link to the workshop material is more valuable since it has more content and is more detailed. If we are adding link, should we add it at the end, saying that more details about it can be found there?
There was a problem hiding this comment.
@adowling2 I have updated the list of contributors based on the ACC workshop page. Let me know if I need to correct anything.
|
@smondal13 we cancelled tests for now so we can divert all testing resources to cutting the release. I'll restart tests on this once we're done. |
| E-optimality we want to maximize the objective function, while for A- and Modified | ||
| E-optimality we want to minimize the objective function. | ||
|
|
||
| However, in this sensitivity analysis plot (heatmap), we only varied the initial |
There was a problem hiding this comment.
@blnicho, following a discussion with @adowling2 regarding the sharp change in the heatmaps at 300K, I've added a new section (L343-369) to clarify this behavior as per @adowling2's suggestion. We agreed this was a critical detail to highlight. I intentionally left out the deep mathematical breakdown to keep the explanation conceptual and accessible for a tutorial.
adowling2
left a comment
adowling2
left a comment
There was a problem hiding this comment.
@smondal13 Minor comments to update a little math. After this change, I think it is ready to merge.
| * - D-optimality | ||
| - :math:`\text{det}({\mathbf{M}})` | ||
| - Volume of the confidence ellipse | ||
| - Volume of the confidence ellipse (derived from the Fisher Information Matrix) |
There was a problem hiding this comment.
These edits are a little confusing. This should just be the volume of the confidence ellipse. det(M) = 1 / det(V) where M = V^{-1}. This is due to properties of eigenvalues
There was a problem hiding this comment.
@adowling2 A confidence ellipse is usually drawn from the covariance matrix. Since we are solving maximization ( max log-determinant ) problem, then in our case, if we use D-optimality = volume of the confidence ellipse, it would mean that we are maximizing the volume of the confidence ellipse drawn from the covariance matrix. That's why I wanted to be clear. What do you think is the best way to avoid that?
There was a problem hiding this comment.
This led to the subsequent changes in the geometric interpretation as well.
There was a problem hiding this comment.
Work out the math on paper with the eigenvalues. This will show the equivalence of the geometric interpretations.
There was a problem hiding this comment.
I know they are equivalent: max log_det (M) is equivalent to min (log_det(M^-1)) for design. That is not what I am worried about.
In line 100, we defined the problem as a maximization problem. So, if we just say "D-optimality is Volume of the confidence ellipse", and then set the problem as a maximization problem, then that would mean we are saying D-optimal criterion is maximizing the volume of the confidence ellipse, which is drawn from the covariance matrix ( V= M^-1). But that would be incorrect since the D-optimality criterion means either maximizing the volume of FIM or minimizing the volume of V=M^-1 (confidence ellipse).
There was a problem hiding this comment.
@adowling2 Following our discussion, I have implemented the suggestion so that it becomes clear to the reader. Please check at your earliest convenience.
|
@smondal13 our testing infrastructure is back up and running. Merge main into your branch to get tests passing. |
Fix formatting and clarify descriptions in doe.rst documentation
blnicho
left a comment
blnicho
left a comment
There was a problem hiding this comment.
@smondal13 thanks for addressing my previous comments and clarifying the heatmaps in the tutorial results. I think this is very close but I did find some small grammatical typos and a couple sections that aren't being rendered on ReadtheDocs correctly. Once those are fixed, this can be merged.
| .. literalinclude:: /../../pyomo/contrib/doe/examples/reactor_example.py | ||
| :start-after: Read in file | ||
| :end-before: End sensitivity analysis | ||
| :language: python | ||
| :start-after: # This software is distributed under the 3-clause BSD License. | ||
| :end-before: if __name__ == "__main__": | ||
| :linenos: |
There was a problem hiding this comment.
This code block isn't rendering in the docs
There was a problem hiding this comment.
It is rendering now
Correct typos and minor grammatical errors Co-authored-by: Bethany Nicholson <blnicho@users.noreply.github.com>
- Added clarity to the documentation regarding the use of the `label_experiment` method. - Replaced doctest with literalinclude for better code example integration in `doe.rst`. - Added missing import section in `reactor_example.py` and removed unnecessary blank lines in `reactor_experiment.py`.
|
@adowling2 - Happy to wait to merge if you'd like to review again. Just let us know! |
github-project-automation
bot
moved this from Ready for final review
to Done
in ParmEst & Pyomo.DoE Development
7 participants
Fixes # .
Summary/Motivation:
Pyomo.DoE documentation was old and needed an update.
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: