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

Code cleanup #100

Merged
merged 4 commits into from
Sep 7, 2021
Merged

Code cleanup #100

merged 4 commits into from
Sep 7, 2021

Conversation

ewu63
Copy link
Collaborator

@ewu63 ewu63 commented Sep 6, 2021

Purpose

This PR cleans up some of the code:

  • used pyupgrade to update some syntax
  • some manual fixes (replaced type() etc.)
  • flake8 fixes
  • docstring updates

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

N/A

Checklist

Put an x in the boxes that apply.

  • I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
  • 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

@ewu63 ewu63 requested a review from a team as a code owner September 6, 2021 13:53
@codecov
Copy link

codecov bot commented Sep 6, 2021

Codecov Report

Merging #100 (9ada80c) into master (9eff5c0) will not change coverage.
The diff coverage is 44.54%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #100   +/-   ##
=======================================
  Coverage   51.12%   51.12%           
=======================================
  Files          40       40           
  Lines        9608     9608           
=======================================
  Hits         4912     4912           
  Misses       4696     4696           
Impacted Files Coverage Δ
pygeo/constraints/areaConstraint.py 78.61% <0.00%> (ø)
pygeo/constraints/circularityConstraint.py 81.57% <0.00%> (ø)
pygeo/constraints/colinearityConstraint.py 88.23% <0.00%> (ø)
pygeo/constraints/locationConstraint.py 78.26% <0.00%> (ø)
pygeo/constraints/radiusConstraint.py 79.38% <0.00%> (ø)
pygeo/constraints/thicknessConstraint.py 85.10% <0.00%> (ø)
pygeo/constraints/volumeConstraint.py 95.65% <0.00%> (ø)
pygeo/geo_utils/index_position.py 66.66% <ø> (ø)
pygeo/pyGeo.py 3.93% <0.00%> (ø)
pygeo/constraints/planarityConstraint.py 83.92% <16.66%> (ø)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9eff5c0...9ada80c. Read the comment docs.

@@ -69,7 +69,7 @@ def evalFunctionsSensFD(self, DVGeo, DVCon, fdstep=1e-2):
funcsSens[outkey][inkey][:, array_ind] = deriv_temp
xDV[inkey][array_ind] = baseVar[array_ind]
DVGeo.setDesignVars(xDV)
DVCon.evalFunctions(dict())
DVCon.evalFunctions({})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've no idea why this line is needed..

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean {} instead of dict()? Or the fact that we are calling DVCon?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh now I see, it's a third call to DVCon at the end, just before exiting the FD sensitivity method.. Maybe it's just to reset the internal values, same as done by DVGeo.setDesignVars() above? I need to check the function

Copy link
Contributor

Choose a reason for hiding this comment

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

@nwu63 I suspect that it is needed because every time we evaluate a constraint we update the coordinates of the pyGeo object. I think that if we directly use DVGeo.update() the tests should work, up to you if you want to play with that.
I might be wrong bc I don't know what error you are getting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I've no idea, but without line 72 one of the tests fail, and if I also take out line 71 then a few more tests fail. I can see cases where 71 is needed (even though it's probably not the best practice), but not sure why 72 is needed at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't tell what that line is doing either, the error I get from taking it out is in Test 8: Circularity constraint and looks like it's not meeting the tolerance. Neil is this what you're seeing?

AssertionError: 
Not equal to tolerance rtol=1e-07, atol=1e-07
Failed value for: circularity_base
Mismatched elements: 8 / 9 (88.9%)
Max absolute difference: 2.15268112e-05
Max relative difference: 2.15268112e-05
 x: array([0.999989, 0.999978, 0.999987, 1.      , 0.999995, 0.99998 ,
       0.999981, 0.999994, 1.      ])
 y: array([1., 1., 1., 1., 1., 1., 1., 1., 1.])

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes exactly.

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.

Looks good, see my comments/questions

pygeo/parameterization/DVGeoAxi.py Show resolved Hide resolved
pygeo/parameterization/DVGeoESP.py Show resolved Hide resolved
pygeo/constraints/DVCon.py Show resolved Hide resolved
@@ -69,7 +69,7 @@ def evalFunctionsSensFD(self, DVGeo, DVCon, fdstep=1e-2):
funcsSens[outkey][inkey][:, array_ind] = deriv_temp
xDV[inkey][array_ind] = baseVar[array_ind]
DVGeo.setDesignVars(xDV)
DVCon.evalFunctions(dict())
DVCon.evalFunctions({})
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh now I see, it's a third call to DVCon at the end, just before exiting the FD sensitivity method.. Maybe it's just to reset the internal values, same as done by DVGeo.setDesignVars() above? I need to check the function

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.

After some offline discussion, I think this is ready to be merged.
That constraint test will really need some overhauling, but it is something that should be done at a later stage

Copy link
Contributor

@hajdik hajdik left a comment

Choose a reason for hiding this comment

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

Updates all look good, agree with fixing the constraint test at a later date.

@ewu63 ewu63 merged commit 26e73f5 into master Sep 7, 2021
@ewu63 ewu63 deleted the pyupgrade branch September 7, 2021 21:29
hajdik pushed a commit to hajdik/pygeo that referenced this pull request Sep 16, 2021
marcomangano pushed a commit that referenced this pull request Sep 16, 2021
* added pygeo test

* comments

* Code cleanup (#100)

* redoing last commits

* adding  .ref

Co-authored-by: Neil Wu <602725+nwu63@users.noreply.github.com>
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.

3 participants