ENHANCEMENT: Implemented cyipopt minimize_ipopt #342
Conversation
* Make uniform shift default false and fix test_core * Allow for non-uniform shift for impedance
* fix tests * fix pioneer
* Pioneer tutorial realizations demo Add frequency array and realization study to beginning of pioneer tutorial * Update pioneer * Clean pioneer and update docs * remove print * Units * Update pioneer plots * Waves before frequencies * Explain less frequencies * Minor updates * Update pioneer * Merge branch 'dev' of https://github.com/sandialabs/WecOptTool into phases_demo
* bug bix : DC and Nyquist frequency should not be devided by two before ifft * Changed td_to_fd to scale single sided frequency components rather than TD signal * minor bug fix from issue332 sandialabs#332
* run CI on PRs against dev branch * revamped tutorial 1, including fix for sandialabs#293 * more tutorial cleanup and editorial changes * more cleanup and incorporated changes in sandialabs#315 * fixed tutorial 2 colormaps * finishing touches * reverted a few accidental changes * fixes as per Jeff's review comments --------- Co-authored-by: Ryan Coe <[email protected]>
* run CI on PRs against dev branch * coppied fundamental utility files * import utilities module * added utilities funtions to tut1 * added bem plot from utils * added bem plot from utils * updated sankey plot * updated check_radiation_damping * cleared outputs * corrected bug * changed Zi to hydro_impedance to be consistent with our variables name python convention * PR review edits * add grid to plots * removed draft functions in utilities.py * typo * Fixed one more typo I found while reviewing Daniel's changes --------- Co-authored-by: Ryan Coe <[email protected]> Co-authored-by: Michael Devin <[email protected]>
There was a problem hiding this comment.
Hi Jorge, thank you for making this PR with the updates to cyipopt. The implementation looks pretty good overall. I left a few small comments and here are some larger comments to get this PR in a spot to be merged:
- We recently shifted our workflow to merge PRs into dev branch. I changed your PR to be merging into dev instead of main but it looks like this led to some merge conflicts. You can usually view these conflicts in the command line or in GitHub desktop and manually sort through the conflicts to keep the correct version of code.
- These changes don't impact the tutorials, right? Please remove any changes to the tutorials from this PR by reverting them back to the original versions.
…abs#337) * post_processing docstrings - examples - parameters (order) * handle multiple phase realizations internally * Update wecopttool/core.py * making outputs lists * Update implementation to function with tutorial 1 for now * Update tutorials * Update LUPA * Make sure same WEC is passed in * Add test_utilities Revert "Add test_utilities" This reverts commit 27399f0. * Update utilities module * Update utilities * Update test_utilities * Update tutorial 1 utilities call --------- Co-authored-by: Carlos A. Michelén Ströfer <[email protected]> Co-authored-by: Carlos A. Michelén Ströfer <[email protected]> Co-authored-by: jtgrasb <[email protected]> Co-authored-by: jtgrasb <[email protected]>
|
I made another commit removing the comments regarding the callback implementation no longer being used and the unnecessary import. I also noticed that there were no changes made to the tutorials, the only thing I did was test two of them and make sure they worked. After that I restarted the Kernel and cleared all outputs, it might be detecting that as a change? |
|
Thanks for making those updates. The tutorials can be a bit finnicky to work with for these PRs. It looks like the outputs are still recorded in the PR here, so you should clear all outputs again and then go to edit notebook metadata and make sure the display name and version are the same as the default. The goal here is that neither of the tutorials show up in the Files Changed section on this PR. |
|
Awesome, thanks for the heads up. It should be correct now. Not showing changes to the tutorials! |
Pull Request Test Coverage Report for Build 9130293491Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
@jtgrasb - Please take a closer at this and compare |
|
According to cyipopt's documentation, the following differences when compared to
When testing the tutorials side-by-side with scipy and cyipopt as currently implemented, there are no observable differences. The first difference stated above is not applied because we are still specifying 'SLSQP' as the solver method. If we leave the method section empty, we can leverage the interior point method, so this is worth exploring here. Further differences may come with a shift to cyipopt's Problem Interface. The problem interface will require a restructuring of how the optimization problem is defined. |
Description
Alternative optimization algorithm implemented: cyipopt minimize_ipopt #12
Type of PR
Checklist for PR