Skip to content

Pyomo.DoE bugfixes #3574

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 21, 2025
Merged

Pyomo.DoE bugfixes #3574

merged 5 commits into from
May 21, 2025

Conversation

djlaky
Copy link
Contributor

@djlaky djlaky commented Apr 23, 2025

Fixes # .

  1. Ensured square solve (e.g., fix the experiment inputs) when solving each scenario block.
  2. Added self.tee to sequential compute FIM method

Summary/Motivation:

Fixing some bugs found in Pyomo.DoE over the past month.

Changes proposed in this PR:

  • Adding fixing/unfixing of experimental design decisions to ensure a square solve for each finite difference scenario
  • Added a reference to the internal tee value in the compute FIM methods

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.

Ensured square solve (e.g., fix the experiment inputs) when solving each scenario block.

Added self.tee to sequential computing FIM solves.
@djlaky
Copy link
Contributor Author

djlaky commented Apr 23, 2025

@smondal13, @adowling2

Bug we saw this morning that was fixed but not pushed.

@adowling2
Copy link
Member

@djlaky Is this ready for review by the Pyomo team?

Copy link
Member

@adowling2 adowling2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

@djlaky
Copy link
Contributor Author

djlaky commented Apr 29, 2025

@djlaky Is this ready for review by the Pyomo team?

Yes. This should be ready for review @blnicho @jsiirola

Comment on lines +1123 to +1125
# Fix experiment inputs before solve (enforce square solve)
for comp in b.experiment_inputs:
comp.fix()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any model checking before this that ensures fixing b.experiment_inputs will result in a square model?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is there any chance that a user will have already fixed some of these inputs in which case we shouldn't unfix them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is not a chance of this. The understanding of Pyomo.DoE is be that all the experiment inputs (as labeled) should be fixed during initialization and free during the optimization call. If they should be fixed, the user should not include them in that suffix.

@adowling2, can you check to make sure what I said is correct?

Copy link
Contributor Author

@djlaky djlaky Apr 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any model checking before this that ensures fixing b.experiment_inputs will result in a square model?

There is not.

Since the blocks are built on cloning the model from 'get_labeled_model()', the model output from this call needs to be square.

Also, is there a way to tell if the model is square using the 'res' object in line 1127? That would be the best case, then the user can get an error if their model is not square and I can write a test in the suite.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of this failing gracefully if the user tries running it with a non-square model. I have less of an opinion on if the check happens here or somewhere else.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we have a custom function that inspects the build model and summarize the number of measurements, equations, parameters, inputs, etc. and estimates the degrees of freedom. I am thinking "If someone contacted me asking for help with ParmEst or Pyomo.DoE, what would I am to know about the model? I'm s there a way we can make the first debugging step to run "model_diagnostics" or similar and report the output.

Copy link

codecov bot commented May 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.83%. Comparing base (18def95) to head (a87930e).
Report is 13 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3574   +/-   ##
=======================================
  Coverage   88.83%   88.83%           
=======================================
  Files         889      889           
  Lines      102205   102209    +4     
=======================================
+ Hits        90792    90797    +5     
+ Misses      11413    11412    -1     
Flag Coverage Δ
builders 26.65% <0.00%> (+<0.01%) ⬆️
default 84.95% <100.00%> (?)
expensive 34.04% <0.00%> (?)
linux 86.61% <100.00%> (-1.96%) ⬇️
linux_other 86.61% <100.00%> (+<0.01%) ⬆️
osx 82.91% <100.00%> (-0.01%) ⬇️
win 84.82% <100.00%> (+<0.01%) ⬆️
win_other 84.82% <100.00%> (+<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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mrmundt mrmundt merged commit d7917b7 into Pyomo:main May 21, 2025
64 of 65 checks passed
@github-project-automation github-project-automation bot moved this from Ready for final review to Done in ParmEst & Pyomo.DoE Development May 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

5 participants