Skip to content
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

Add function to PETSc interface to return initial condition problem #1443

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

dallan-keylogic
Copy link
Contributor

Fixes

Summary/Motivation:

When using the PETSc interface, a square initial condition problem is first solved (with either PETSc-snes or IPOPT) to make sure the initial condition is consistent before sending integrations problems to PETSc-TS. However, when you end up with degree of freedom problems or a degenerate problem, it's difficult to debug. As an expedient, I would edit the PETSc problem to return the problem in the middle of the context manager.

I've added a function to get this problem directly and modified the PETSc interface to use this function to get the initial condition problem to solve to ensure consistency.

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  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.

@Robbybp
Copy link
Member

Robbybp commented Jun 26, 2024

This looks really useful, thanks for making the change. I'll give it a review soon.

@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2024

Codecov Report

Attention: Patch coverage is 80.88235% with 13 lines in your changes missing coverage. Please review.

Project coverage is 76.36%. Comparing base (10b42e4) to head (6850cb2).
Report is 25 commits behind head on main.

Files with missing lines Patch % Lines
idaes/core/solvers/petsc.py 80.88% 7 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1443      +/-   ##
==========================================
- Coverage   76.36%   76.36%   -0.01%     
==========================================
  Files         394      394              
  Lines       64953    64993      +40     
  Branches    14404    14412       +8     
==========================================
+ Hits        49601    49630      +29     
- Misses      12793    12803      +10     
- Partials     2559     2560       +1     

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

@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Jul 11, 2024
@lbianchi-lbl
Copy link
Contributor

@Robbybp, just a friendly nag to review when (or if) you have some time.

@ksbeattie ksbeattie requested review from adam-a-a and removed request for dangunter, blnicho, lbianchi-lbl and bpaul4 August 8, 2024 18:43
@dallan-keylogic
Copy link
Contributor Author

@Robbybp , could you please review this sometime soon? I'd really like it to make the August release.

Copy link
Member

@Robbybp Robbybp left a comment

Choose a reason for hiding this comment

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

Questions/comments/requests about error handling and function arguments.

Comment on lines +1102 to +1111
def get_initial_condition_problem(
model,
time,
initial_time,
representative_time=None,
initial_constraints=None,
initial_variables=None,
detect_initial=True,
flattened_problem=None,
):
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 reason initial_time is required? I would think that time.first() would be a reasonable default.

Comment on lines +1148 to +1155
if flattened_problem is None:
if representative_time is None:
raise RuntimeError(
"The user must supply either the flattened problem or a representative time."
)
flattened_problem = _get_flattened_problem(
model=model, time=time, representative_time=representative_time
)
Copy link
Member

Choose a reason for hiding this comment

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

Why is either a flattened problem or representative time required? Shouldn't this function just use the same defaults that petsc_dae_by_time_element uses?

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'm trying to avoid having to flatten the problem multiple times. I haven't profiled it, but I suspect it's rather slow. I debated reimplementing petsc_dae_by_time_element as an Object so that the flattened problem can be cached. Maybe that will happen eventually.

Copy link
Member

Choose a reason for hiding this comment

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

I have no problem with a flattened_problem argument, I just think if neither it nor representative time is provided, we should default to _get_flattened_problem(model=model, time=time, representative_time=time.at(2)).

Comment on lines +1133 to +1134
detect_initial (bool): If True, add non-time-indexed variables and
constraints to initial_variables and initial_constraints.
Copy link
Member

Choose a reason for hiding this comment

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

I don't love the name of this option, but since it is the same as in petsc_dae_by_time_element, I think it should stay.

Comment on lines +1135 to +1136
flattened_problem (dict): Dictionary returned by get_flattened_problem.
If not provided, get_flattened_problem will be called at representative_time.
Copy link
Member

Choose a reason for hiding this comment

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

I assume this option is provided so we don't have the overhead of recomputing the flattened model when calling within petsc_dae_by_time_element? I'm fine with this option, but would prefer for it to not be part of this function's public API. This is mostly because I don't want a user to have to construct a dict with undocumented keys (or use the private _get_flattened_problem function) to use this option. I recommend renaming the option to _flattened_problem, and adding to the docstring that its behavior is subject to change.

For future applications where a "flattened model" is necessary, the DynamicModelInterface from pyomo.contrib.mpc may be useful (see https://github.com/Pyomo/pyomo/blob/404fd6d997d24f7209e8af4c427a13d6e10f19c4/pyomo/contrib/mpc/interfaces/model_interface.py#L52 or https://pyomo.readthedocs.io/en/stable/contributed_packages/mpc/interface.html#pyomo.contrib.mpc.interfaces.model_interface.DynamicModelInterface).

Comment on lines +1177 to +1181
if len(constraints) <= 0:
raise RuntimeError(
"Zero constraints in initial condition problem, therefore "
"there is no block to return."
)
Copy link
Member

Choose a reason for hiding this comment

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

We could just return an empty block, or None, in this case. I think I like this better than raising an error here that we then catch above. We already check the number of constraints before trying to solve the block, so an empty block should be fine, although None might be more explicit.

Comment on lines +597 to +611
try:
init_subsystem = get_initial_condition_problem(
model=m,
time=time,
initial_time=t0,
initial_constraints=initial_constraints,
initial_variables=initial_variables,
detect_initial=detect_initial,
flattened_problem=flattened_problem,
)
except RuntimeError as err:
if "Zero constraints" in err.message:
init_subsystem = None
else:
raise err
Copy link
Member

Choose a reason for hiding this comment

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

See below. I'd rather this function return None, and we handle it here, than rely on parsing the error message.

@ksbeattie
Copy link
Member

Progress not expected until end of Sept (per @dallan-keylogic)

@dangunter dangunter added Priority:Low Low Priority Issue or PR and removed Priority:Normal Normal Priority Issue or PR labels Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Low Low Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants