idaes-pse icon indicating copy to clipboard operation
idaes-pse copied to clipboard

[RFC] Have initialize methods return final solver status

Open andrewlee94 opened this issue 4 years ago • 7 comments

It came up today that we currently have no method of confirming solver status a the end of initialization routines. For one, this limits us when writing tests for these routines, as we have no way to check that they actually solved correctly.

One solution to this would be to have initialize methods return the final solver status at the end, which we could then checks as normal. However, this would be an API change and would require many models to be updated to handle the additional output.

Currently, property and control volume initialize methods return a single dict (flags indicating which state variables were left fixed and need to be unfixed later). Adding an additional output would require:

  1. Updating all models which use assign the returned values to accept two values instead of 1.
  2. Having a default value for the flags return, and returning it in all cases. Currently initialize methods generally return None if no state vars are left fixed.
  3. Making sure none of the above breaks anything else.

So, this is not a trivial thing that can be slipped in without anyone noticing, and this would require a bit of tedium to update everything.

Update:

Have started adding new InitializaitonErrors to all models. To Do:

  • [x] core property packages #618
  • [x] generic unit models #627
  • [ ] distillation column models
  • [ ] power plant models
  • [ ] gas-solids contactor models

andrewlee94 avatar May 21 '21 17:05 andrewlee94

As a less disruptive alternative, we could add and initialized flag to the models. Maybe in the ProcessBlockData class. Give it an enumerated type like NOT_SUPPORTED, NOT_RUN, SUCCESS, FAIL. Not supported could could be the default, in case some models haven't done anything with it yet. It won't interfere with the initialize methods that already return stuff for other purposes.

eslickj avatar May 21 '21 19:05 eslickj

Commenting on this PR just to mention that I would appreciate this feature as well. Will think more about if there are any clever ways of implementing it.

dallan-keylogic avatar Dec 02 '21 15:12 dallan-keylogic

@eslickj Whilst attaching the initialization status to the model would avoid the API issues, I don't really like the idea attaching information like that to the model. For one, it will become out of date as soon as the next solver call is made - i.e., it only tells us that the model was initialized at some point, but not when.

Another possibility would be to shoe-horn the solver status into the existing flags, but that would involve mixing two different types of information into one dict (doable, but maybe not good practice).

andrewlee94 avatar Dec 02 '21 16:12 andrewlee94

@andrewlee94, this is a really unpleasant suggestion, but I've wondering about using @Robbybp's context manager that allows you to temporarily fix certain variables or get submodels for initialization. I know it would be a huge pain to update things, but if we would break the API for this, we could consider lumping those together.

eslickj avatar Dec 02 '21 18:12 eslickj

Noting outcome of discussion at Dev Team call: it was decided to have initialization routines raise an Exception if the final step fails. Initialization routines could optionally raise an Exception earlier if desired.

andrewlee94 avatar Dec 02 '21 18:12 andrewlee94

@andrewlee94 any update on this?

ksbeattie avatar Jul 28 '22 18:07 ksbeattie

This is an on going discussion. A few of us think it is a good idea, but it needs a lot of planning.

andrewlee94 avatar Jul 28 '22 18:07 andrewlee94

This has been rolled into the new initialization API: #1051

andrewlee94 avatar Jan 05 '23 19:01 andrewlee94