Pyomo.DoE adds checking sentences to the type of objects #3250

jialuw96 · 2024-04-26T00:33:20Z

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:

I agree my contributions are submitted under the BSD license.
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 · 2024-04-27T17:35:08Z

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

pyomo/contrib/doe/doe.py

jialuw96 · 2024-05-08T05:16:21Z

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.

pyomo/contrib/doe/doe.py

codecov · 2024-05-08T20:45:59Z

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 ⚠️
pyomo/contrib/doe/doe.py	55.00%	9 Missing ⚠️

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%)`	⬆️
osx	`?`
other	`86.62% <81.30%> (-0.01%)`	⬇️
win	`83.94% <81.30%> (-0.01%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Co-authored-by: Bethany Nicholson <blnicho@users.noreply.github.com>

blnicho

@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 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

blnicho · 2024-05-09T21:22:12Z

pyomo/contrib/doe/doe.py

-                # If fix_opt is True, fix the design variable
-                var.fix(design_val[name])
-            else:
-                # Otherwise check optimize_option
-                if optimize_option is None:
-                    # If optimize_option is None, unfix all design variables
-                    var.unfix()
+
+            if var.ctype is Var:
+                if fix_opt:
+                    var.fix(design_val[name])
                else:
-                    # Otherwise, unfix only the design variables listed in optimize_option with value True


Why did you remove the comments explaining the logic here?

This is added back

blnicho · 2024-05-09T21:28:28Z

pyomo/contrib/doe/examples/reactor_compute_FIM.py

+        formula="central",  # formula for finite difference
+    )
+
+    result2.result_analysis()


Are you expecting the results to be the same as the previous case? If so I think you should have the same assert statements checking the trace and determinant.

I modified this test to use both Param components for parameters and variables, change one parameter value, use assertions to check if the generated model uses the updated model parameter value, and check the trace and determinant.

blnicho · 2024-05-09T21:45:27Z

pyomo/contrib/doe/doe.py

            for par in self.param:
                cuid = pyo.ComponentUID(par)
                var = cuid.find_component_on(b)


There is another copy of this code starting on line 612. Does it need to be updated also?

Yes, they are updated now

blnicho · 2024-05-09T21:52:51Z

pyomo/contrib/doe/doe.py

+            elif var.ctype is Param:
+                var = design_val[name]


What if this is called with fix_opt=False? Are there cases where the user expects var to be a degree of freedom and they would be surprised by this code silently leaving it as fixed? If this is expected behavior then I think it needs to be clear in the documentation what the tradeoff is when using a design variable declared as a Pyomo Param vs a Pyomo Var.

if with fix_opt=False, var will be unfixed, so it is fine if the user wants to leave it as DOF (they can use fix_opt=False to do that). I add a statement in the doc to note that, if a design variable is defined as Param, stochastic_program function will not work because it cannot be optimized.

jialuw96 · 2024-05-10T18:50:38Z

@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!

adowling2 · 2024-05-28T02:07:23Z

@blnicho Is this ready for review again?

adowling2 · 2024-05-29T14:15:27Z

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.

jialuw96 and others added 3 commits April 18, 2024 11:59

updated branch for RPB model

f452a16

Merge branch 'Pyomo:main' into RPB_MBDOE

47abeb2

run black

12693a9

mrmundt reviewed Apr 30, 2024

View reviewed changes

pyomo/contrib/doe/doe.py Outdated Show resolved Hide resolved

jialuw96 added 2 commits April 30, 2024 17:40

fix typo

fd99272

run black

e2e9e68

mrmundt requested changes May 1, 2024

View reviewed changes

pyomo/contrib/doe/doe.py Outdated Show resolved Hide resolved

jialuw96 added 4 commits May 4, 2024 20:27

add test

e253de7

run black

5f852d5

fix the simplevar bug

4c67380

debugged T[0.125] is not IndexedVar but _GeneralVar problem; add tests

0f0fb7f

lbianchi-lbl mentioned this pull request May 7, 2024

rotary bed notebooks CCSI-Toolset/fixed_bed_adsorption#3

Open

jialuw96 and others added 4 commits May 7, 2024 18:44

sync to main fork

3938082

Merge branch 'main' into RPB_MBDOE

2eb2567

rerun black

b9ff9e1

remove unneeded file

3bcebd5

mrmundt added the AT: PRE-TEST INSPECTED label May 8, 2024

mrmundt approved these changes May 8, 2024

View reviewed changes

pyomo-autotest removed the AT: PRE-TEST INSPECTED label May 8, 2024

blnicho requested changes May 8, 2024

View reviewed changes

jialuw96 and others added 7 commits May 8, 2024 22:22

Update pyomo/contrib/doe/doe.py

1387955

Co-authored-by: Bethany Nicholson <blnicho@users.noreply.github.com>

Update pyomo/contrib/doe/doe.py

c380672

Co-authored-by: Bethany Nicholson <blnicho@users.noreply.github.com>

Update pyomo/contrib/doe/doe.py

bd8ed3f

Co-authored-by: Bethany Nicholson <blnicho@users.noreply.github.com>

Update pyomo/contrib/doe/doe.py

1bf111f

Co-authored-by: Bethany Nicholson <blnicho@users.noreply.github.com>

Update pyomo/contrib/doe/doe.py

e434fb5

Co-authored-by: Bethany Nicholson <blnicho@users.noreply.github.com>

Update pyomo/contrib/doe/doe.py

98d759f

Co-authored-by: Bethany Nicholson <blnicho@users.noreply.github.com>

Merge branch 'main' into RPB_MBDOE

a11527a

jialuw96 added 2 commits May 8, 2024 22:41

Use fix() instead of setlb and setub to Var

5e761e0

Fix test error

eb3b753

mrmundt added the AT: PRE-TEST INSPECTED label May 9, 2024

pyomo-autotest removed the AT: PRE-TEST INSPECTED label May 9, 2024

blnicho requested changes May 9, 2024

View reviewed changes

jialuw96 added 2 commits May 9, 2024 22:30

fix var = number to var.value = number; and comments

ba466c2

update comments and tests

ac10aad

adowling2 and others added 2 commits May 28, 2024 08:19

Merge branch 'main' into RPB_MBDOE

655c0e6

Restoring comments in doe.py

2e990e2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pyomo.DoE adds checking sentences to the type of objects #3250

Pyomo.DoE adds checking sentences to the type of objects #3250

jialuw96 commented Apr 26, 2024

jialuw96 commented Apr 27, 2024

jialuw96 commented May 8, 2024

codecov bot commented May 8, 2024 •

edited

blnicho left a comment •

edited

blnicho May 9, 2024

jialuw96 May 10, 2024

blnicho May 9, 2024

jialuw96 May 10, 2024

blnicho May 9, 2024

jialuw96 May 10, 2024

blnicho May 9, 2024 •

edited

jialuw96 May 10, 2024

jialuw96 commented May 10, 2024

adowling2 commented May 28, 2024

adowling2 commented May 29, 2024

Pyomo.DoE adds checking sentences to the type of objects #3250

Are you sure you want to change the base?

Pyomo.DoE adds checking sentences to the type of objects #3250

Conversation

jialuw96 commented Apr 26, 2024

Fixes # .

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

jialuw96 commented Apr 27, 2024

jialuw96 commented May 8, 2024

codecov bot commented May 8, 2024 • edited

Codecov Report

blnicho left a comment • edited

Choose a reason for hiding this comment

blnicho May 9, 2024

Choose a reason for hiding this comment

jialuw96 May 10, 2024

Choose a reason for hiding this comment

blnicho May 9, 2024

Choose a reason for hiding this comment

jialuw96 May 10, 2024

Choose a reason for hiding this comment

blnicho May 9, 2024

Choose a reason for hiding this comment

jialuw96 May 10, 2024

Choose a reason for hiding this comment

blnicho May 9, 2024 • edited

Choose a reason for hiding this comment

jialuw96 May 10, 2024

Choose a reason for hiding this comment

jialuw96 commented May 10, 2024

adowling2 commented May 28, 2024

adowling2 commented May 29, 2024

codecov bot commented May 8, 2024 •

edited

blnicho left a comment •

edited

blnicho May 9, 2024 •

edited