Save more variables by default in SNOPT history file

[ewu63]

Purpose

I updated the snSTOP function to save more variables by default in the Python layer. The option Save major iteration variables now defaults to an empty list. The following variables are saved (the user cannot turn these off)

• isMajor
• nMajor
• nMinor
• step
• feasibility
• optimality
• merit
• condZHZ
• penalty

and the following can be added via the option

• Hessian
• slack
• lambda
• condZHZ
• nS
• BSwap
• maxVi
• penalty_vector

Happy to discuss names/which ones should be saved by default. I think it's possible to have everything in the major iteration log saved, but the defaults I feel are the ones people actually care about (at least given my limited knowledge of SNOPT).

Expected time until merged

A few days. Maybe we can do a minor release after this and #346 are both merged?

Type of change

• 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)

I don't believe this is a backwards-breaking change, but happy to discuss. Also please run tests since the CI will not check SNOPT-related stuff.

Testing

I updated the existing tests. As usual, the CI will not test SNOPT so please test this locally.

Checklist

• I have run flake8 and black to make sure the Python code adheres to PEP-8 and is consistently formatted
• I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
• 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]

Codecov Report

Merging #347 (adfb0b6) into main (b029888) will decrease coverage by 11.41%.
The diff coverage is 18.18%.

@@             Coverage Diff             @@
##             main     #347       +/-   ##
===========================================
- Coverage   84.02%   72.61%   -11.41%     
===========================================
  Files          22       22               
  Lines        3349     3345        -4     
===========================================
- Hits         2814     2429      -385     
- Misses        535      916      +381     

Impacted Files	Coverage Δ
pyoptsparse/pySNOPT/pySNOPT.py	16.81% <18.18%> (-73.28%)	⬇️

... and 8 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[sseraj]

I think an error would be better here

[ewu63]

I thought about it, and decided on warning since I don't want to kill a whole optimization just because someone made a typo or had a duplicate variable. Can change if preferred.

[sseraj]

Thoughts on this @lamkina?

[lamkina]

I would rather this be an error with a more descriptive message. If you are expecting to have variables to postprocess afer an optimization, you don't want to have to re-run the entire optimization (especially for high-fidelity cases) just to get that information. Especially because most users initially won't understand this warning and might just ignore the warning.

An error with a message like Received unkown SNOPT save variable {saveVar}. Please see "Save major iteration variables" option in the pyOptSparse documentation under "SNOPT".

[sseraj]

I think it would be better to add to the default list than have variables that cannot be turned off. This would make the code easier to understand, and it would be consistent with how we handle saving variables in ADflow.

[ewu63]

I also debated about this. I think the reason I preferred this is because, if I want to add another one then I have to copy/paste the list of defaults first from somewhere (probably docs) which is a little annoying. Also, given the small storage requirement I don't think it's necessary to allow users to remove the default ones. Happy to change things though.

[sseraj]

What do you think @lamkina?

[lamkina]

I tend to agree with @nwu63 in this case. The storage requirement for the default variables is pretty insignificant and from an OptView perspective it's nice to have an expected minimal set of data for every SNOPT case.

From the user perspective, I'm always against extra steps to get simple cases working. If the proposed alternative means users need to read the docs to understand the default usage of recoring, I think that is too complicated for the intended impact of these changes.

[sseraj]

Alright, that fine with me. I would like the defaults to be documented though because the empty list suggests nothing is saved by default.

[lamkina]

That's a good compromise imo.

[ewu63]

Bumping this PR @sseraj @lamkina

[lamkina]

See my comment about changing the warning to an error. Once that's resolved I think the rest of this is looking good to go.

[marcomangano]

Sorry for my late review, I agree with the comments above. It's not really a new feature and there are no big changes in behavior so I don't think a new release is necessary just for this PR, but we might think about a version bump soon.