Simplify _Q_opt in parmest with block scenario structure. #3789

sscini · 2025-11-20T16:57:51Z

Fixes # .

Summary/Motivation:

_Q_opt, the main model building and solving function within parmest, is still heavily dependent on MSISPPY, and the code is becoming outdated and difficult to interpret. The goal of this PR is to redesign _Q_opt to work without this dependence, retaining all the current functionality

Changes proposed in this PR:

-_Q_opt redesign, using a block structure, with minimal changes needed to functions that utilize it.

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.

… Q_opt-redesign

sscini · 2025-11-20T16:59:01Z

@djlaky @adowling2 Here are my initial thoughts on the redesign. Please provide feedback on code layout to this point.

sscini · 2025-11-20T21:22:06Z

Dan and I had a good conversation today about the functional things needed in the redesign, and how to go about adding components to the overall block and sub-models. Working to implement changes, closely related to what is done currently in doe's _generate_scenario_blocks.

adowling2

Nice progress

pyomo/contrib/parmest/parmest.py

sscini · 2025-11-25T17:34:02Z

@adowling2 @djlaky Added case for bootstrap, now works with theta_est, theta_est_bootstrap, and theta_est_leaveNout if I replace _Q_opt with _Q_opt_blocks. Please review.

I am aware of the duplicated code, but when I attempted to unify them using n_scenarios = len(self.exp_list) or len(bootlist), I am getting an error for bootstrap I am still working out.

Would we want to fully transition to using theta_est for obj and theta, and cov_est for covariance and add an error/warning? Or should I make it work to calculate cov if calc_cov=True?

Thanks!

codecov · 2025-11-30T19:59:10Z

Codecov Report

❌ Patch coverage is 4.76190% with 100 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.31%. Comparing base (13facca) to head (d91ce3f).
⚠️ Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
pyomo/contrib/parmest/parmest.py	4.76%	100 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3789      +/-   ##
==========================================
- Coverage   89.40%   89.31%   -0.10%     
==========================================
  Files         909      909              
  Lines      105541   105646     +105     
==========================================
- Hits        94364    94360       -4     
- Misses      11177    11286     +109

Flag	Coverage Δ
builders	`29.07% <4.76%> (-0.04%)`	⬇️
default	`85.94% <4.76%> (?)`
expensive	`35.73% <4.76%> (?)`
linux	`86.63% <4.76%> (-2.55%)`	⬇️
linux_other	`86.63% <4.76%> (-0.10%)`	⬇️
osx	`82.78% <4.76%> (-0.09%)`	⬇️
win	`84.88% <4.76%> (-0.09%)`	⬇️
win_other	`84.88% <4.76%> (-0.09%)`	⬇️

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.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sscini · 2025-12-09T17:36:48Z

@adowling2 @djlaky Should I tag larger team on this? Please review when available. Thanks!

…ocks" This reverts commit 1aea99f.

sscini · 2026-01-15T04:25:04Z

@adowling2 @slilonfe5 Ready for review. All functionalities should be working. Two small things to discuss, but relating to covariance calculation in _cov_at_theta. Thank you!

…_list) cov_est() needs the experiment class to have variables for params, which makes a few tests fail. Was failing tests with one experiment.

adowling2

@sscini Here is my first pass at the review. A handful of places would benefit from more verbose comments to explain the logic or assumptions.

I wonder if there is a better way besides getattr to access variable values. I am thinking how we access values in Pyomo.DoE for variables that are in a suffix.

@slilonfe5, please take a look. I think this would benefit from a conversation with you to integrate the different methods to estimate the parameter covariance.

adowling2 · 2026-01-20T17:44:35Z

pyomo/contrib/parmest/examples/reaction_kinetics/simple_reaction_parmest_example.py

    model.x2 = Param(initialize=float(data['x2']))

    # Rate constants
+    # @Reviewers: Can we switch this to explicitly defining which parameters are to be


Yes, I think this example could/should be reworked to fully embrace the Experiment class.

adowling2 · 2026-01-20T17:47:11Z

pyomo/contrib/parmest/examples/reactor_design/parameter_estimation_example.py


    # Parameter estimation with covariance
-    obj, theta, cov = pest.theta_est(calc_cov=True, cov_n=17)
+    obj, theta, cov = pest.theta_est(calc_cov=True, cov_n=19)


Just to satisfy my curiosity, is 17 incorrect and 19 correct? Is this a mistake in the example that is being fixed?

From the data in the csv file, it should be 19. There are 19 experiments and 4 experiment outputs. The experiment counter (_count_total_experiments) is currently returning 76, which is why I have mentioned making it more robust and commented out the related assertion at this time.

adowling2 · 2026-01-20T17:47:58Z

pyomo/contrib/parmest/examples/reactor_design/parameter_estimation_example.py

    # Parameter estimation with covariance
-    obj, theta, cov = pest.theta_est(calc_cov=True, cov_n=17)
+    obj, theta, cov = pest.theta_est(calc_cov=True, cov_n=19)
    print(obj)


While we are editing this, let's print out something such as "print("Least squares objective = ",obj)", etc. to help a new user.

adowling2 · 2026-01-20T18:06:28Z

pyomo/contrib/parmest/examples/reactor_design/reactor_design.py

-    )  # min^-1
-    model.k3 = pyo.Param(
-        initialize=1.0 / 6000.0, within=pyo.PositiveReals, mutable=True
+    # Rate constants, make unknown parameters variables


Should this example get refactored to use the Experiment class? Is this something @smondal13 is thinking about for a separate PR?

adowling2 · 2026-01-20T18:08:06Z

pyomo/contrib/parmest/parmest.py

 #### Redesign with Experiment class Dec 2023

+# Options for using mpi-sppy or local EF only used in the deprecatedEstimator class
 # TODO: move use_mpisppy to a Pyomo configuration option


Does this need to get cleaned up? Is this for the deprecated interface only?

Will be cleaned up. Yes, currently the use_mpisppy option is only included in deprecated interface. This was added before I started this PR.

adowling2 · 2026-01-20T18:32:23Z

pyomo/contrib/parmest/parmest.py

+            for exp_i in exp_blocks:
+                vals = {}
+                for var in return_values:
+                    exp_i_var = exp_i.find_component(str(var))


What is this doing? More comments would help? Is it assuming specific things about the structure of the parmest model? At a minimum, I would recommend documenting any assumptions in comments at least.

adowling2 · 2026-01-20T18:33:36Z

pyomo/contrib/parmest/parmest.py

+            var_values = pd.DataFrame(var_values)
+
+        # Calculate covariance if requested using cov_est()
+        if calc_cov is not NOTSET and calc_cov:


When would calc_cov be assigned NOTSET?

[Addresses next few comments]. Need to confirm with @blnicho and team which tests need to be kept and enforced with these changes. If we call cov_est or _cov_at_theta here, then we are restraining the user to only using variables for unknown parameters, which makes multiple of the existing tests and examples fail. The inverse reduced hessian method was what @slilonfe5 implemented to calculate the covariance if a user still wishes to use theta_est for covariance. Previous conversations indicated these features should be preserved from my understanding.
Will also add more comments

adowling2 · 2026-01-20T18:35:23Z

pyomo/contrib/parmest/parmest.py

-                return obj_val, theta_vals, cov
-            elif calc_cov is NOTSET or not calc_cov:
-                return obj_val, theta_vals
+            measurement_var = self.obj_value / (


How does this work with the changes @slilonfe5 introduced to define different objectives?

adowling2 · 2026-01-20T18:36:54Z

pyomo/contrib/parmest/parmest.py

-                    return obj_val, theta_vals, var_values, cov
-                elif calc_cov is NOTSET or not calc_cov:
-                    return obj_val, theta_vals, var_values
+            solve_result, inv_red_hes = (


Should this call the _cov_at_theta function?

adowling2 · 2026-01-20T18:38:11Z

pyomo/contrib/parmest/parmest.py

                    1
                )  # initialization performed using just 1 set of theta values
+
+        # print("DEBUG objective_at_theta_blocks")


Remove these commented out print statements before we merge. They are okay to keep while debugging / working on other feedback.

slilonfe5 · 2026-01-20T19:02:37Z

@sscini Here is my first pass at the review. A handful of places would benefit from more verbose comments to explain the logic or assumptions.

I wonder if there is a better way besides getattr to access variable values. I am thinking how we access values in Pyomo.DoE for variables that are in a suffix.

@slilonfe5, please take a look. I think this would benefit from a conversation with you to integrate the different methods to estimate the parameter covariance.

Hello @adowling2, I agree. Stephen and I have been talking a lot about integrating the covariance estimation, as well as help with understanding the parmest test file. I will review and leave comments and suggestions.

sscini · 2026-01-20T19:06:18Z

Notes to discuss in 01/20/26 meeting:

Does the use of parameters and indexed parameters still need to be preserved?
The _count_total_experiments function: Suggestions for path forward.

slilonfe5 · 2026-01-20T19:16:11Z

Notes to discuss in 01/20/26 meeting:

Does the use of parameters and indexed parameters still need to be preserved? The _count_total_experiments function: Suggestions for path forward.

Hello @sscini. I think you want your code to be robust to those old parmest examples, we can discuss later. I will leave suggestions on the _count_total_experiments function.

slilonfe5 · 2026-01-21T15:54:31Z

pyomo/contrib/parmest/parmest.py



+# Need to make this more robust. Used in Estimator class
+# Has issue where it counts duplicate data if multiple non-unique outputs


Suggested change

# Has issue where it counts duplicate data if multiple non-unique outputs

total_number_data = 0

for experiment in experiment_list:

# get the experiment outputs

output_variables = experiment.get_labeled_model().experiment_outputs

# get the parent component of the first output variable

parent = list(output_variables.keys())[0].parent_component()

# check if there is only one unique experiment output, e.g., dynamic output variable

if all(v.parent_component() is parent for v in output_variables):

total_number_data += len(output_variables)

else:

total_number_data += 1

return total_number_data

@sscini suggested code to fix the issue. I checked, and this works better.

slilonfe5 · 2026-01-21T16:03:03Z

pyomo/contrib/parmest/parmest.py

-        calc_cov=NOTSET,
-        cov_n=NOTSET,
-    ):
+    def _create_scenario_blocks(self, bootlist=None, ThetaVals=None, fix_theta=False):


@sscini I think you need the calc_cov and cov_n arguments. It allows users who use theta_est for covariance calculation to continue doing so. If the user doesn't pass the calc_cov and cov_n arguments in theta_est, then calc_cov=NOTSET and cov_n=NOTSET.

sscini added 4 commits November 17, 2025 21:15

First pass at code redesign, still need to figure out more

10f6b37

Merge branch 'Pyomo:main' into Q_opt-redesign

b0b7e2c

Added comments where I have question

3e95e91

Merge branch 'Q_opt-redesign' of https://github.com/sscini/pyomo into…

8d3a4d5

… Q_opt-redesign

sscini added 2 commits November 21, 2025 01:35

Got preliminary _Q_opt simple working with example!

3982e1b

Ran black

e829344

sscini added this to ParmEst & Pyomo.DoE Development Nov 21, 2025

sscini moved this to Development in ParmEst & Pyomo.DoE Development Nov 21, 2025

adowling2 reviewed Nov 21, 2025

View reviewed changes

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

sscini added 5 commits November 21, 2025 09:36

Changed name to _Q_opt_blocks

e464097

Update parmest.py

dc5ee76

Ran black

6355818

Added in case for bootlist, works with example

099f541

Ran black

76ee05e

Simplified structure, ran black

d91ce3f

jsiirola added the AT: PRE-TEST INSPECTED label Nov 30, 2025

pyomo-autotest removed the AT: PRE-TEST INSPECTED label Nov 30, 2025

blnicho added the ParmEst label Dec 11, 2025

sscini added 7 commits December 13, 2025 15:09

Removed _Q_opt, and replicate functions, only using _Q_opt_blocks

1aea99f

Ran black on mac

7d93cc0

Revert "Removed _Q_opt, and replicate functions, only using _Q_opt_bl…

d7d2214

…ocks" This reverts commit 1aea99f.

Added testing statement

7f21344

Ran black

32d8d41

Merge branch 'Pyomo:main' into Q_opt-redesign

4e8c3ee

Made small design changes, in progress, ran black.

1e802ba

Changed back to get_labeled_model in _cov_at_theta()

3261798

sscini added 6 commits January 14, 2026 23:25

Added notes in unused files.

fc478be

Removed _Q_at_theta and objective_at_theta

acba985

Added comments for reviewers, ran black.

145c2d8

Corrected count_total_experiments to divide by # outputs.

337095d

Ran black.

837192c

Undo change to count_total_experiments.

4b46c30

blnicho marked this pull request as ready for review January 15, 2026 21:37

sscini added 5 commits January 19, 2026 13:17

Update mpi_utils.py

b9cf010

Switched unknown_params to Vars

062a9ee

Fixed number for cov_n, still need to adjust counting function

5baaa2f

Added question for reviewers

c8194ac

Ran black

26d70e3

sscini requested a review from adowling2 January 19, 2026 18:22

sscini added 7 commits January 19, 2026 14:37

Changed retrieval of variables for ind_red_hes.

4aa027d

Added note related to count_total_experiments, commented out assertion.

26ba2ea

Ran black

dd926f8

Removed dependence on cov_est for theta_est(), added bool for len(exp…

7b70d1d

…_list) cov_est() needs the experiment class to have variables for params, which makes a few tests fail. Was failing tests with one experiment.

Merge branch 'main' into Q_opt-redesign

43cbaa4

Ran black

07798c9

Attempted adding support for indexed vars

b325f0d

adowling2 suggested changes Jan 20, 2026

View reviewed changes

sscini added 2 commits January 20, 2026 14:31

Ran black

8b47430

Addressed some review comments.

aac1476

slilonfe5 reviewed Jan 21, 2026

View reviewed changes



		# Need to make this more robust. Used in Estimator class
		# Has issue where it counts duplicate data if multiple non-unique outputs

-# Has issue where it counts duplicate data if multiple non-unique outputs
+total_number_data = 0
+for experiment in experiment_list:
+    # get the experiment outputs
+    output_variables = experiment.get_labeled_model().experiment_outputs
+    # get the parent component of the first output variable
+    parent = list(output_variables.keys())[0].parent_component()
+    # check if there is only one unique experiment output, e.g., dynamic output variable
+    if all(v.parent_component() is parent for v in output_variables):
+        total_number_data += len(output_variables)
+    else:
+        total_number_data += 1
+return total_number_data

Simplify _Q_opt in parmest with block scenario structure. #3789

Are you sure you want to change the base?

Simplify _Q_opt in parmest with block scenario structure. #3789

Conversation

sscini commented Nov 20, 2025

Fixes # .

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

Uh oh!

sscini commented Nov 20, 2025

Uh oh!

sscini commented Nov 20, 2025

Uh oh!

adowling2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sscini commented Nov 25, 2025

Uh oh!

codecov bot commented Nov 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sscini commented Dec 9, 2025

Uh oh!

sscini commented Jan 15, 2026

Uh oh!

adowling2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sscini Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slilonfe5 commented Jan 20, 2026

Uh oh!

sscini commented Jan 20, 2026

Uh oh!

slilonfe5 commented Jan 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Nov 30, 2025 •

edited

Loading

sscini Jan 20, 2026 •

edited

Loading