Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Edited plotting file to cater for all the optimizers. Added a main fu… #158

Merged
merged 5 commits into from
Jun 12, 2024

Conversation

umarsajjad1992
Copy link
Contributor

@umarsajjad1992 umarsajjad1992 commented Jun 7, 2024

Purpose

Update the plotting file for optimization history to cater for all the optimizers in the current version

Description

  • The plotting file only catered for SNOPT optimizer and would give errors when using SLSQP
  • An issue was raised and a suggestion of a PR to correct this behavior
  • Add a parser argument with opt option to check for optimizer and then run the code accordingly
  • Added an if else statement to check which optimizer is being used
  • The code would change the layout and number of the plots accordingly
  • Added a try and except block to check whether an optimization history file exists or not

Expected time until merged

The PR is not urgent as, but a week should be enough for review and merging.

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

run the code:

python plot_optHist.py --histFile output/opt.hst --outputDir output/ --opt "SNOPT"

try changing the --opt argument to either "SLSQP" or "IPOPT"

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

@umarsajjad1992 umarsajjad1992 requested a review from a team as a code owner June 7, 2024 04:34
Copy link
Contributor

@marcomangano marcomangano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening this PR! Looking good, I was initially confused by the choice of a large try...except block but I see you are gracefully exiting at the end, makes sense.
I will shortly push a commit to fix the black and flake8 errors. @hajdik it looks like an ESP test is failing, can you check what is going on?

tutorial/opt/aero/plot_optHist.py Outdated Show resolved Hide resolved
tutorial/opt/aero/plot_optHist.py Outdated Show resolved Hide resolved
tutorial/opt/aero/plot_optHist.py Outdated Show resolved Hide resolved
tutorial/opt/aero/plot_optHist.py Outdated Show resolved Hide resolved
- Removed try..except block for simple os.path.isfile check
- Removed rcParams and parser lines from inside of main
- Changed args.opt to hist.getMetadata()['optimizer']
- Removed code duplication of plotting
ewu63
ewu63 previously approved these changes Jun 7, 2024
@ewu63
Copy link
Collaborator

ewu63 commented Jun 7, 2024

I'm pretty sure the failure is just because we're running SNOPT tests and this is a PR submitted by an external contributor without access to that optimizer (hence this PR...ironically). May want to add skiptest or something similar to what we do with pyOptSparse tests.

@marcomangano
Copy link
Contributor

I'm pretty sure the failure is just because we're running SNOPT tests and this is a PR submitted by an external contributor without access to that optimizer (hence this PR...ironically). May want to add skiptest or something similar to what we do with pyOptSparse tests.

You were right, the ESP test was missing the skiptest. Merging the PR once I verify that the tests pass.

@ewu63 ewu63 linked an issue Jun 10, 2024 that may be closed by this pull request

if __name__ == "__main__":
# plt.rcParams["text.usetex"] = True # Comment out if latex installation is not present
plt.rcParams["font.family"] = "serif"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally prefer having all plt commands before any function definition, at the module level. Not blocking this PR though, since this is probably personal preference.

@marcomangano marcomangano merged commit 7151de3 into mdolab:main Jun 12, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues in Plotting of Optimization History
3 participants