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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3812 +/- ##
==========================================
- Coverage 89.41% 89.41% -0.01%
==========================================
Files 909 909
Lines 105579 105579
==========================================
- Hits 94408 94407 -1
- Misses 11171 11172 +1
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.
blnicho
left a comment
blnicho
left a comment
There was a problem hiding this comment.
I'm about halfway through my review but here are some initial comments.
| - :math:`\text{trace}({\mathbf{M}}^{-1})` | ||
| - Dimensions of the enclosing box of the confidence ellipse | ||
| * - Pseudo A-optimality | ||
| - :math:`\text{trace}({\mathbf{M}})` | ||
| - Dimensions of the enclosing box of the inverse of the confidence ellipse |
There was a problem hiding this comment.
Are these descriptions swapped by mistake?
There was a problem hiding this comment.
No, these changes were intentional. This goes back to #3803 . We have added a new mathematical definition where we call the trace of FIM pseudo A-optimality or pseudo-trace, and the trace of the inverse of FIM as the trace
There was a problem hiding this comment.
That part I follow. I'm asking about the "Geometrical Meaning" column. It doesn't look like the term "confidence ellipse" is ever defined or related back to M in the docs so I wanted to make sure those were still correct.
There was a problem hiding this comment.
Yes, it was never defined. I have added clarification and added a note to clarify what is meant by confidence ellipse.
Co-authored-by: Bethany Nicholson <blnicho@users.noreply.github.com>
blnicho
left a comment
blnicho
left a comment
There was a problem hiding this comment.
I started refactoring the text describing the example to follow the structure of the Parmest Quick Start Guide but I noticed that the design space heatmaps look off so I think this still needs some substantial work.
There was a problem hiding this comment.
Why is does the top row of this plot (T=300) look strange? The optimality criteria is positive there but negative everywhere else? The previous plots from this example didn't have this artifact.
There was a problem hiding this comment.
That top row at T=300K represents the worst design in the space. Because we are minimizing the A-optimality criteria, the maximum positive value is highly unfavorable. Physically, this makes perfect sense: the example uses the Arrhenius equation, so at a low temperature like 300K, the reaction rate drops significantly, yielding very little information for the design.
Regarding the difference from older plots: since this is strictly a documentation PR, I haven't modified any underlying model parameters. I simply generated this updated plot using the current state of the codebase. The previous plots in the docs were likely generated using an older version of the model before recent updates to the main branch. I see that the .png file in doe.rst was last changed 1year ago, but the doe/examples/result.json which is used in the example script was changed 7 months ago, and there was a change in control_points. So my understanding is that this or something else was not captured in the stable documentation. Finally, the 'sudden' visual jump at 300K is just a discretization artifact; the transition would look much smoother if we plotted this over a finer temperature grid rather than the current coarse steps.
There was a problem hiding this comment.
the 'sudden' visual jump at 300K is just a discretization artifact; the transition would look much smoother if we plotted this over a finer temperature grid rather than the current coarse steps.
Could you run a quick test to confirm this? Could you include both 300K and 305K in the heatmap and paste the figure here?
There was a problem hiding this comment.
The plotting function uses np.linspace, so to make the computation quick, I have used the range of temperature from 300K to 350K and used 6 points. I think it will help with the idea that the jump in value is actually not a jump, but a gradual change.
There was a problem hiding this comment.
I have also added points from 300K -> 305K; this will make it clearer.
There was a problem hiding this comment.
Same comment here about the top row. Something seems off in these results.
There was a problem hiding this comment.
Same comment here about the top row. Something seems off in these results.
There was a problem hiding this comment.
Same comment here about the top row. Something seems off in these results.
6 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: