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

Fixed IPOPT history recording #239

Closed
wants to merge 3 commits into from
Closed

Conversation

kanekosh
Copy link
Contributor

@kanekosh kanekosh commented Apr 26, 2021

Purpose

Addresses #182 . This fixes the bug that IPOPT history files constraint duplicated entries in a hacky way.

Note

It basically checks if the current call is already recorded or not based on x values. If the current x is identical to the previous x at which we wrote the history, then we don't write the history.
I first wanted to check that based on the type of the call (i.e. constraints or objective), then write the history only when it is an objective (or its gradient) call. This worked fine for cold starts and the code would've been much cleaner, but it is problematic when we parse the history for hot starts.
When we don't record the constraint calls, during the hot start, we need to get info from an adjacent call counter (which is the objective call we recorded at the same x). But the problem is that we don't know which adjacent call counter (+1 or -1) we should refer, because we don't know which function (objective or constraints) IPOPT calls first at each iteration. It sometimes calls the objective first, but the other times it calls the constraints first. That's why we have to rely onx values.

Since the code is hacky, it might be better to keep the history file unfixed, but delete the duplicated entries in OptView or somewhere. What do you think @nwu63 @A-Gray-94 ?

Type of change

What types of change is it?
Select the appropriate type(s) that describe this PR

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (please describe)

Testing

This is the history file of test_hs015.py. It matches what IPOPT claims (12 function and 11 jacobian evaluations). The previous behavior can be found in #238 .
Screenshot from 2021-04-25 23-20-56

Checklist

Put an x in the boxes that apply.

  • I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

@kanekosh
Copy link
Contributor Author

kanekosh commented May 7, 2021

Alternative fix in #252

@kanekosh kanekosh deleted the ipopt_his branch June 8, 2023 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant