pyoptsparse icon indicating copy to clipboard operation
pyoptsparse copied to clipboard

Added restartDict to snstop

Open sseraj opened this issue 1 year ago • 4 comments

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)
  • [x] 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

  • [x] 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
  • [x] I have run unit and regression tests which pass locally with my changes
  • [x] I have added new tests that prove my fix is effective or that my feature works
  • [x] I have added necessary documentation

sseraj avatar May 31 '24 21:05 sseraj

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). Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pyoptsparse/pySNOPT/pySNOPT.py 81.81% 2 Missing :warning:
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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 31 '24 21:05 codecov[bot]

Maybe also do a version bump here (minor?)

ewu63 avatar Jun 05 '24 17:06 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.

ewu63 avatar Jun 05 '24 20:06 ewu63

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

sseraj avatar Jun 14 '24 04:06 sseraj

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.

ewu63 avatar Jul 09 '24 01:07 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

marcomangano avatar Jul 10 '24 10:07 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

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.

ewu63 avatar Jul 10 '24 17:07 ewu63