Added restartDict to snstop

[sseraj]

Purpose

I added an option to save restartDict after each major iteration. This is helpful when you want to restart optimizations after they crash unexpectedly or run out of time. In the latter case, this avoids the guesswork of setting a conservative time limit. I also added an option that allows restartDict to be passed to snstop_handle if the user wants to access this after every major iteration.

Expected time until merged

1-2 weeks

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)

Testing

I added a test that writes restartDict using snstop and uses the dictionary to restart the optimization. I also tested this with my aerodynamic shape optimization cases.

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

Attention: Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 74.92%. Comparing base (d96a398) to head (19a3fd7).

Files	Patch %	Lines
pyoptsparse/pySNOPT/pySNOPT.py	81.81%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #404      +/-   ##
==========================================
+ Coverage   74.88%   74.92%   +0.03%     
==========================================
  Files          22       22              
  Lines        3325     3334       +9     
==========================================
+ Hits         2490     2498       +8     
- Misses        835      836       +1     

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

[ewu63]

Looks good, like you said this is a breaking change but probably not too bad for downstream users. Another design pattern I had considered previously was for the user to tell pyOptSparse which information to request back from snstop, and those are returned (either as a single dict or just a bunch of args). That would be the most flexible but will require some further implementation.

[ewu63]

TODO for the future: this should probably be a dataclass object instead of a dict... That way we can avoid some code duplication with above.

[VascoSch92]

@ewu63 I can do it if you open an issue with the specification of what you would like to have ;-)

[ewu63]

Opened #405

[sseraj]

Thanks for the comments. It would be nice for the function to be more flexible. I'm not sure how returning values would work though because the user does not call _snstop. It seems like the dictionaries have to be passed in.

[ewu63]

Maybe also do a version bump here (minor?)

[ewu63]

Thanks for the comments. It would be nice for the function to be more flexible. I'm not sure how returning values would work though because the user does not call _snstop. It seems like the dictionaries have to be passed in.

The way I'm thinking is the same as how we implemented Save major iteration variables. Basically, pass in a list of what you want, and snstop will be called with those args, matching the order. By default we can always pass iterDict.

Another thought - we can also make the "save work arrays at every iter" an option, instead of asking the user to make the snstop function. We can add an option save work arrays which by default is False or None, but you can set a path and then the built-in snstop function can store these (in some preferred format). This will standardize the restarts.

[sseraj]

This is ready for another review. I implemented @ewu63's suggestions so this is more useable and is no longer a breaking change.

[ewu63]

I think the version bump got swallowed in the merge resolution. Otherwise LGTM, thanks!

[marcomangano]

Just a couple of minor comments, looks great. I will bump the minor version in a minute, but I would like to have #410 merged before this so we can include that in the new release.

[ewu63]

So I forgot that we made it not a breaking change, meaning we should've bumped patch and not minor version... oh well a bit late now, I'd rather not leave any commits on main with incorrect version numbers.

[marcomangano]

So I forgot that we made it not a breaking change, meaning we should've bumped patch and not minor version... oh well a bit late now, I'd rather not leave any commits on main with incorrect version numbers.

I bumped the minor version because this is technically a new feature, I think we got a bit loose with version numbers tbh. Not a big deal, but we can be a bit more strict in the future

[ewu63]

So I forgot that we made it not a breaking change, meaning we should've bumped patch and not minor version... oh well a bit late now, I'd rather not leave any commits on main with incorrect version numbers.

I bumped the minor version because this is technically a new feature, I think we got a bit loose with version numbers tbh. Not a big deal, but we can be a bit more strict in the future

In general I have shied away from semver when I was in the lab, and instead stuck largely to Numpy's versioning scheme, for the reasons outlined there. There is nothing inherently wrong with bumping the minor version frequently, but it should carry some weight behind it - otherwise we quickly move to v2.52.0 without actual substantial changes to the software.