IPOPT duplicated history handling #252

kanekosh · 2021-05-07T23:21:35Z

Purpose

Closes #182. I added iter entries to the history data at every call counter. Other than that the hist file is unchanged.
Then the duplicated entries are removed in OptView_baseclasses and History.getValues().
When we read the old hist files which don't have the iter entries, OptView and getValues() raise a warning.

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

The history file and OptView plot for test_hs015.py. IPOPT.out says it had 12 function evaluations (= iter).

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

codecov · 2021-05-07T23:25:15Z

Codecov Report

Merging #252 (184d238) into master (bf140e2) will decrease coverage by 10.83%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master     #252       +/-   ##
===========================================
- Coverage   83.44%   72.61%   -10.84%     
===========================================
  Files          22       22               
  Lines        3250     3268       +18     
===========================================
- Hits         2712     2373      -339     
- Misses        538      895      +357

Impacted Files	Coverage Δ
pyoptsparse/pyoptsparse/pySNOPT/pySNOPT.py	`13.86% <0.00%> (-76.24%)`	⬇️
pyoptsparse/pyoptsparse/pyNLPQLP/pyNLPQLP.py	`25.92% <0.00%> (-66.67%)`	⬇️
pyoptsparse/pyoptsparse/pyOpt_solution.py	`57.77% <0.00%> (-42.23%)`	⬇️
pyoptsparse/pyoptsparse/pyOpt_utils.py	`61.86% <0.00%> (-7.91%)`	⬇️
pyoptsparse/pyoptsparse/pyOpt_history.py	`76.44% <0.00%> (-5.26%)`	⬇️
pyoptsparse/pyoptsparse/pyOpt_optimizer.py	`83.29% <0.00%> (-0.97%)`	⬇️
pyoptsparse/pyoptsparse/pyOpt_optimization.py	`78.01% <0.00%> (-0.82%)`	⬇️
pyoptsparse/pyoptsparse/pyOpt_error.py	`92.00% <0.00%> (+36.00%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf140e2...184d238. Read the comment docs.

kanekosh · 2021-05-08T00:42:37Z

I don't know why flake8 is failing, looks like F821 is pyNSGA2/setup.py? Also isort error was fine on my local machine...

ewu63 · 2021-05-08T00:54:47Z

Don't worry about flake8 and isort, I changed the configuration today but only applied the fixes to PR #251 rather than to the master branch. I will try to fix it, but worst case we can merge this regardless.

ewu63

Just some comments, the main thing I would like to fix is how the callCounter loop is done in History. A few other things:

We definitely want to add a test for this, but given that I am working on refactoring the tests in parallel maybe that needs to happen in a separate PR later.
The docs page history.rst need to be updated, to add the iter key to the ASCII diagram showing history file structure.

pyoptsparse/pyOpt_optimizer.py

ewu63 · 2021-05-08T13:58:49Z

pyoptsparse/pyOpt_history.py

@@ -630,29 +630,32 @@ def getValues(self, names=None, callCounters=None, major=True, scale=False, stac
            callCounters.append(self.read("last"))
            callCounters.remove("last")

+        self._ipoptIterCounter = -1  # track iteration, only relevant for IPOPT


I'm not a fan of this approach here. Can we go through a pre-processing step to determine the "suitable" callCounters to loop over in the main loop instead? We should be able to come up with an approach that does not depend on checking the optimizer, and is generally applicable to all history files.

Current method should work for other optimizers as well (will double check), I just put IPOPT in the variable/method names so that what I intend here is clear. But I will rename those.
I don't know if we want to have another pre-processing loop, as we are already doing some callCounter validation in the main loop (func vs funcSense, major, fail)

Yeah I was thinking of creating a loop before we parse the file, where we generate the list of callCounters given all the various input flags. Then we only loop over those. But maybe that would decrease the efficiency so maybe it's not worth it. I'm okay with keeping it as is, but maybe some minor refactor would be helpful:

lump all the checks into a single function that validates the callCounter against things like funcs, major flags etc

make it general for all optimizers

pyoptsparse/postprocessing/OptView_baseclass.py

pyoptsparse/pyOpt_history.py

kanekosh · 2021-05-08T15:48:50Z

I don't have a good idea for the test other than comparing the final iterCounter with the hardcoded number of iterations we get from each optimizers' output file. But it'll be painful to retrain the ref data (because someone has to go through all the output files manually).
Do you have any suggestions?

ewu63 · 2021-05-09T15:06:00Z

I don't have a good idea for the test other than comparing the final iterCounter with the hardcoded number of iterations we get from each optimizers' output file. But it'll be painful to retrain the ref data (because someone has to go through all the output files manually).
Do you have any suggestions?

I think it's fine to do that for one test and for just IPOPT. We can have another one to test that getValues gets you the correct values. We can work on this in a separate PR or something given the ongoing refactor.

kanekosh · 2021-05-10T16:18:07Z

Ready for another review.
I add a new method _generateValidCallCounters() which does all the call counter checks. This follows the same logical flow (I tested with old and new hist files from IPOPT and SNOPT), but we may want to wait to merge until adding new tests?

ewu63

What do you think? Would the code still be readable?

pyoptsparse/postprocessing/OptView_baseclass.py

ewu63 · 2021-05-11T15:23:05Z

pyoptsparse/pyOpt_history.py

@@ -630,40 +630,26 @@ def getValues(self, names=None, callCounters=None, major=True, scale=False, stac
            callCounters.append(self.read("last"))
            callCounters.remove("last")

-        self._ipoptIterCounter = -1  # track iteration, only relevant for IPOPT
+        # get a list of valid call counters
+        validCallCounters = self._generateValidCallCounters(callCounters, user_specified_callCounter, allowSens, major)


Now that I am looking at this again, I think this may have a significant performance penalty since we are looping over the database twice. Could we have a function that checks if the current callCounter is valid? We would call that at every iteration, and only proceed if valid.

Updated, yeah I think this is better now

ewu63

Looks good to me, just some minor comments. I will wait for others to take a look too. Thanks for this work!

pyoptsparse/pyOpt_history.py

ewu63 · 2021-05-11T19:21:26Z

pyoptsparse/pyOpt_history.py

        return data

+    def _readValidCallCounter(self, i, user_specified_flag, allowSens_flag, major_flag):


Please add docstrings

marcomangano

Every time I look into pyOptSparse I learn something new that I completely ignored before - i.e. the duplicated func and sens calls that are managed within the History() class.

So, if I get it right (aside the warnings for "old" hst files) you added the iter key to facilitate the process of picking the "right" stored iter - namely the one with the function evaluation - by checking against the cached value of the DV vector. This cached value is currently used only to ensure we are not calling the function evaluation unnecessarily within the masterFunc() method. A few comments:

I am getting a bit confused with the major/minor iteration definition as I am used to SNOPT, here we are just talking about func eval vs fcon/sens eval?
for @nwu63: I don't get (even in our current approach) how we squeeze all the info into the major iteration. Are we doing it at all - i.e. saving the sensitivities values within a major iter - or are we just discarding that info? I might be confused again by SNOPT which has the optimality key for every iter
looks like _readValidCallCounter mostly overlaps with Pointexist(), maybe we can get rid of that?
I have further questions about some low-level machinery in Optimizer() now that we scratched it with this cache thing, but that is a separate discussion

Thanks a lot for putting this together! Questions aside, I would also feel safer if @sseraj takes a look at this too

kanekosh · 2021-05-12T00:48:55Z

you added the iter key to facilitate the process of picking the "right" stored iter - namely the one with the function evaluation - by checking against the cached value of the DV vector.

Yes, and iter can be helpful in case users look into the hist file by themselves?

I am getting a bit confused with the major/minor iteration definition as I am used to SNOPT, here we are just talking about func eval vs fcon/sens eval?

Minor iterations are internal iterations for solving the QP subproblems I believe.

looks like _readValidCallCounter mostly overlaps with Pointexist(), maybe we can get rid of that?

Pointexist() only checks if the call counter is within the range of the hist file, _readValidCallCounter() checks a few other things as well. Also Pointexist() is a public method so it might be being called by some other code, so we don't want to change it.

sseraj

Looks pretty good

pyoptsparse/postprocessing/OptView_baseclass.py

pyoptsparse/pyOpt_history.py

marcomangano · 2021-05-12T14:48:18Z

Yes, and iter can be helpful in case users look into the hist file by themselves?

Yeah this seems an improvement beyond the IPOPT issue itself

Minor iterations are internal iterations for solving the QP subproblems I believe.

Exactly, here the difference is between major (i.e. new x) and the other stored calls for fcon anf g, sounds good!

Pointexist() only checks if the call counter is within the range of the hist file, _readValidCallCounter() checks a few other things as well. Also Pointexist() is a public method so it might be being called by some other code, so we don't want to change it.

Good point on the method being public. The snippet itself is pretty small so I agree that touching it would be pretty unnecessary right now

marcomangano · 2021-05-12T14:57:14Z

I think once Sabet's comments are addressed this is good to go!

sseraj

Should we merge #251 first?

ewu63 · 2021-05-13T01:55:19Z

Should we merge #251 first?

Yes I think that'd be best, then we can add some new tests without causing merge conflicts.

marcomangano · 2021-05-15T22:44:32Z

@kanekosh we just merged the test refactoring PR. The only thing missing for this PR to be merged is the additional test for IPOPT stored values, do you think you can address this soon?

kanekosh · 2021-05-17T01:55:52Z

will do this week

check that the iteration counters are correct
check that getValues() does not return the duplicated entry.

…rcouter

marcomangano · 2021-05-18T16:58:44Z

The test looks good, it makes sense to split it from the main parameterized now. Any more actionables here, aside #259 and #250 ?

ewu63 · 2021-05-18T17:20:48Z

test/test_hs015.py

+
+        # Check iteration counters
+        hist = History(self.histFileName, flag="r")
+        data_init = hist.getValues(names=["iter"], callCounters=[0], allowSens=True)


Could you add a similar test via the read function? I want to make sure that the history file is "correct" and the read function should be used instead of getValues since it does a bunch of other things.

Replaced getValues with read for the iterchecks.
I didn't replace it for the second part (where I loop over the iteration and check the consecutive objective values) because we do want to test getValues there.

Yep looks good to me

marcomangano · 2021-05-18T18:23:06Z

test/test_hs015.py

        self.assertEqual(0, data_init["iter"])
-        data_last = hist.getValues(names=["iter"], callCounters=["last"], allowSens=True)
+        data_last = hist.read(hist.read("last"))


Why is this duplicated? you just get the int counter with the first call?

What do you mean? hist.read("last") returns the last call counter (integer)

makes sense!

marcomangano

Good to go! Do we want to address the issues with getValues() before a new release?

kanekosh added 6 commits May 7, 2021 16:46

added iteration counter

a1db04e

need to support old hist files

42e8b76

OptView fix

5c60fb0

added warning to History.getValues()

c5e09b5

Merge branch 'master' of github.com:mdolab/pyoptsparse into itercouter

335b188

fixed getValues()

e0c1e86

kanekosh requested a review from a team as a code owner May 7, 2021 23:21

kanekosh requested review from ewu63 and Xiaosong2105 May 7, 2021 23:21

kanekosh mentioned this pull request May 7, 2021

Fixed IPOPT history recording #239

Closed

12 tasks

ewu63 requested changes May 8, 2021

View reviewed changes

kanekosh added 3 commits May 10, 2021 11:02

comment/warning update

40728c7

refactored getValues()

b4c7286

docs update

2e5c595

ewu63 reviewed May 11, 2021

View reviewed changes

minor fix in getValues

ca3b779

ewu63 requested changes May 11, 2021

View reviewed changes

added docstrings

9c8a5a2

ewu63 requested review from marcomangano and sseraj May 11, 2021 20:48

ewu63 previously approved these changes May 11, 2021

View reviewed changes

marcomangano reviewed May 12, 2021

View reviewed changes

sseraj reviewed May 12, 2021

View reviewed changes

pyoptsparse/postprocessing/OptView_baseclass.py Show resolved Hide resolved

pyoptsparse/postprocessing/OptView_baseclass.py Show resolved Hide resolved

pyoptsparse/pyOpt_history.py Outdated Show resolved Hide resolved

comment update

6092b62

kanekosh dismissed ewu63’s stale review via 6092b62 May 12, 2021 23:42

sseraj previously approved these changes May 13, 2021

View reviewed changes

Merge branch 'master' into itercouter

b03ec52

fix isort

ef80faa

ewu63 dismissed sseraj’s stale review via ef80faa May 16, 2021 17:01

kanekosh added 3 commits May 18, 2021 09:31

Merge branch 'master' of github.com:mdolab/pyoptsparse into itercouter

060f565

added IPOPT test

5a32843

Merge branch 'itercouter' of github.com:kanekosh/pyoptsparse into ite…

1b5443c

…rcouter

ewu63 reviewed May 18, 2021

View reviewed changes

kanekosh added 2 commits May 18, 2021 13:53

use hist.read()

89ddbe1

minor edit

184d238

ewu63 approved these changes May 18, 2021

View reviewed changes

marcomangano reviewed May 18, 2021

View reviewed changes

marcomangano approved these changes May 18, 2021

View reviewed changes

marcomangano merged commit 9e45e5e into mdolab:master May 18, 2021

kanekosh deleted the itercouter branch June 8, 2023 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IPOPT duplicated history handling #252

IPOPT duplicated history handling #252

kanekosh commented May 7, 2021

codecov bot commented May 7, 2021 •

edited

kanekosh commented May 8, 2021

ewu63 commented May 8, 2021

ewu63 left a comment

ewu63 May 8, 2021

kanekosh May 8, 2021

ewu63 May 9, 2021

kanekosh May 10, 2021

kanekosh commented May 8, 2021

ewu63 commented May 9, 2021 •

edited

kanekosh commented May 10, 2021

ewu63 left a comment

ewu63 May 11, 2021

kanekosh May 11, 2021

ewu63 left a comment

ewu63 May 11, 2021

marcomangano left a comment •

edited

kanekosh commented May 12, 2021

sseraj left a comment

marcomangano commented May 12, 2021

marcomangano commented May 12, 2021

sseraj left a comment

ewu63 commented May 13, 2021

marcomangano commented May 15, 2021 •

edited

kanekosh commented May 17, 2021 •

edited

marcomangano commented May 18, 2021

ewu63 May 18, 2021

kanekosh May 18, 2021

ewu63 May 18, 2021

marcomangano May 18, 2021

kanekosh May 18, 2021

marcomangano May 18, 2021

marcomangano left a comment

		return data

		def _readValidCallCounter(self, i, user_specified_flag, allowSens_flag, major_flag):

IPOPT duplicated history handling #252

IPOPT duplicated history handling #252

Conversation

kanekosh commented May 7, 2021

Purpose

Type of change

Testing

Checklist

codecov bot commented May 7, 2021 • edited

Codecov Report

kanekosh commented May 8, 2021

ewu63 commented May 8, 2021

ewu63 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanekosh commented May 8, 2021

ewu63 commented May 9, 2021 • edited

kanekosh commented May 10, 2021

ewu63 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewu63 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcomangano left a comment • edited

Choose a reason for hiding this comment

kanekosh commented May 12, 2021

sseraj left a comment

Choose a reason for hiding this comment

marcomangano commented May 12, 2021

marcomangano commented May 12, 2021

sseraj left a comment

Choose a reason for hiding this comment

ewu63 commented May 13, 2021

marcomangano commented May 15, 2021 • edited

kanekosh commented May 17, 2021 • edited

marcomangano commented May 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcomangano left a comment

Choose a reason for hiding this comment

codecov bot commented May 7, 2021 •

edited

ewu63 commented May 9, 2021 •

edited

marcomangano left a comment •

edited

marcomangano commented May 15, 2021 •

edited

kanekosh commented May 17, 2021 •

edited