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

fixed sorting of evalFuncs when passed as kwargs #249

Merged
merged 1 commit into from
Dec 6, 2022
Merged

fixed sorting of evalFuncs when passed as kwargs #249

merged 1 commit into from
Dec 6, 2022

Conversation

marcomangano
Copy link
Contributor

Purpose

The evalFuncs argument in evalFunctions() and evalFunctionsSens() is casted to a sorted list in several parts of baseclasses.AeroProblem and pyAdflow, except when the functions are passed as kwarg in pyADflow.evalFuncs() - see this line.
This creates issues when the evalFuncs kwarg is passed as a set in a multi-processor run. Python sets indeed do not preserve ordering within the iterable object, so when the function values are gathered from different procs in a for loop over the list index, the values of different functions will be mixed together.

This PR addresses this by casting the problematic list to a sorted list.

Expected time until merged

Not much, I do not think a test is necessary here.

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

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

@marcomangano marcomangano requested a review from a team as a code owner December 6, 2022 01:44
@ewu63
Copy link
Collaborator

ewu63 commented Dec 6, 2022

For reference, this is the same issue that has appeared before when we first ported things to py3. See for example mdolab/baseclasses#3 (one of my earliest PRs!)

I don't think this is the best approach though. I guess I never considered that the user would pass in a set, but if we are going to accept that, I would propose casting the input to a sorted list instead of what's in this PR. E.g. after line #1356

else:
    evalFuncs = sorted(list(evalFuncs))

which is the same approach taken in baseclasses and other repos.

@codecov
Copy link

codecov bot commented Dec 6, 2022

Codecov Report

Merging #249 (1c2bfe1) into main (069ab31) will increase coverage by 12.45%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##             main     #249       +/-   ##
===========================================
+ Coverage   28.84%   41.30%   +12.45%     
===========================================
  Files          13       13               
  Lines        3806     3806               
===========================================
+ Hits         1098     1572      +474     
+ Misses       2708     2234      -474     
Impacted Files Coverage Δ
adflow/pyADflow.py 68.43% <100.00%> (+21.33%) ⬆️
adflow/MExt.py 97.36% <0.00%> (+18.42%) ⬆️

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

@marcomangano
Copy link
Contributor Author

For reference, this is the same issue that has appeared before when we first ported things to py3. See for example mdolab/baseclasses#3 (one of my earliest PRs!)

I don't think this is the best approach though. I guess I never considered that the user would pass in a set, but if we are going to accept that, I would propose casting the input to a sorted list instead of what's in this PR. E.g. after line #1356

else:
    evalFuncs = sorted(list(evalFuncs))

which is the same approach taken in baseclasses and other repos.

Nvm, we figured that sorted() already casts the set into a list. We could refactor the loop right above in a more compact way though.

Copy link
Collaborator

@sseraj sseraj left a comment

Choose a reason for hiding this comment

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

Nice work getting to the bottom of this

@anilyil anilyil merged commit b4ceb9a into main Dec 6, 2022
@anilyil anilyil deleted the sortFix branch December 6, 2022 14:45
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.

4 participants