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

Fix sol.fStar for NSGA2 #330

Merged
merged 10 commits into from
Feb 27, 2023
Merged

Fix sol.fStar for NSGA2 #330

merged 10 commits into from
Feb 27, 2023

Conversation

ewu63
Copy link
Collaborator

@ewu63 ewu63 commented Feb 25, 2023

Purpose

This PR fixes the sol.fStar value for NSGA2. Basically, the optimized f value was not being passed to createSolution.

Expected time until merged

2 days

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

An associated test is added which fails without the fix.

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
Copy link

codecov bot commented Feb 25, 2023

Codecov Report

Merging #330 (92deed6) into main (21a4f67) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #330      +/-   ##
==========================================
+ Coverage   84.22%   84.25%   +0.03%     
==========================================
  Files          22       22              
  Lines        3340     3347       +7     
==========================================
+ Hits         2813     2820       +7     
  Misses        527      527              
Impacted Files Coverage Δ
pyoptsparse/pyNSGA2/pyNSGA2.py 87.36% <100.00%> (+0.70%) ⬆️
pyoptsparse/pyOpt_optimizer.py 85.36% <100.00%> (+0.06%) ⬆️

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

marcomangano
marcomangano previously approved these changes Feb 25, 2023
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.

Discussed offline, LGTM

marcomangano
marcomangano previously approved these changes Feb 27, 2023
@@ -188,6 +188,12 @@ def objconfunc(nreal, nobj, ncon, x, f, g):
for i in range(n):
xstar[i] = nsga2.doubleArray_getitem(x, i)

if len_ff > 1:
for i in range(len_ff):
ff[i] = nsga2.doubleArray_getitem(f, i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add some comments to explain what's happening here? I don't understand why ff has to be overwritten.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I am not mistaken, ff is just initialized (as a 0-length array) in _assembleObjective. Before this fix, fStar was never updated with the value of the optimal objective f, returned from the actual optimizer called by nsga2.nsga2().
Moreover, f is a swig object so nsga2.doubleArray_getitem is necessary to convert it to a numpy array.

@sseraj sseraj merged commit bed467e into main Feb 27, 2023
@sseraj sseraj deleted the fix-nsga-sol branch February 27, 2023 17:11
@marcomangano marcomangano mentioned this pull request Feb 27, 2023
13 tasks
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.

None yet

3 participants