pyomo icon indicating copy to clipboard operation
pyomo copied to clipboard

Pyomo.DoE adds checking sentences to the type of objects

Open jialuw96 opened this issue 1 year ago • 6 comments

Fixes # .

No issue

Summary/Motivation:

In Pyomo.DoE, check if an object is Var or Param before proceeding to fix it or set bounds to it.

Changes proposed in this PR:

  • Check if an object is Var before setting upper and lower bounds to it. If it is not a type of Var, no need to fix it.
  • Check if an object is Var before fixing it to a given value. If it is one type of Param, check if it is mutable before changing it value.

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 Apr 26 '24 00:04 jialuw96

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

jialuw96 avatar Apr 27 '24 17:04 jialuw96

The unit test is added in the following file:

  • contrib/doe/examples/reactor_compute_FIM.py

We added the unit test where in the kinetics example, the design variables are defined as Param instead of Var, to test if the checking sentences can identify the type of the objects and fix them to another value.

jialuw96 avatar May 08 '24 05:05 jialuw96

Codecov Report

Attention: Patch coverage is 81.30841% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 88.47%. Comparing base (a0765b4) to head (3bcebd5). Report is 199 commits behind head on main.

Files Patch % Lines
pyomo/contrib/doe/examples/reactor_kinetics.py 86.74% 11 Missing :warning:
pyomo/contrib/doe/doe.py 55.00% 9 Missing :warning:
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3250   +/-   ##
=======================================
  Coverage   88.47%   88.47%           
=======================================
  Files         852      852           
  Lines       96222    96319   +97     
=======================================
+ Hits        85131    85223   +92     
- Misses      11091    11096    +5     
Flag Coverage Δ
linux 86.43% <81.30%> (+<0.01%) :arrow_up:
osx ?
other 86.62% <81.30%> (-0.01%) :arrow_down:
win 83.94% <81.30%> (-0.01%) :arrow_down:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 08 '24 20:05 codecov[bot]

@jialuw96 could you explain the motivation behind this PR? It looks like the Pyomo.DoE API was meant to support design variables specified as either Pyomo mutable Param or Var components but there were a few bugs with the Param case. Is that right?

The current Pyomo.DoE API can only support design variables and to-be-estimated parameters as Var components; This PR allows the user to specify them as either Var or Param components.

The docstrings for the DesignOfExperiments and DesignVariables classes should also be updated. Right now they don't mention that both Param and Var components are accepted: design_vars – A DesignVariables which contains the Pyomo variable names and their corresponding indices and bounds for experiment degrees of freedom

This is updated. Thank you!

jialuw96 avatar May 10 '24 18:05 jialuw96

@blnicho Is this ready for review again?

adowling2 avatar May 28 '24 02:05 adowling2

Decision: @adowling2 will work on resolving conflicts and extensive testing after #3267 is merged. This current PR is NOT critical for the Pyomo.DoE workshop next week.

adowling2 avatar May 29 '24 14:05 adowling2

These changes are being superseded by #3317

blnicho avatar Jul 17 '24 13:07 blnicho