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.

Sign up for GitHub

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

Jump to bottom

Formatting and linting fixes #77

Merged
merged 19 commits into from
Apr 2, 2021
Merged

Formatting and linting fixes #77

merged 19 commits into from
Apr 2, 2021

Conversation

ewu63
Copy link
Collaborator

@ewu63 ewu63 commented Mar 30, 2021

Purpose

Closes #54, I used black to format the repo and then fixed all linting errors. Please check the commits carefully, I will also leave comments on specific changes I made to highlight those that I wanted someone to double check.

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

No new tests, all existing tests pass.

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 March 30, 2021 18:56
@ewu63 ewu63 requested review from bbrelje and anilyil March 30, 2021 18:56
ewu63
ewu63 commented Mar 30, 2021
View reviewed changes
pygeo/DVConstraints.py Outdated Show resolved Hide resolved
ewu63
ewu63 commented Mar 30, 2021
View reviewed changes
pygeo/DVGeometry.py
@@ -3571,8 +3571,8 @@ def _spanwiselocalDVJacobian(self, config=None):

# TODO: the += here is to allow recursion check this with multiple nesting
# levels
self.children[iChild].dXrefdXdvl[:, iDVLocal] += dXrefdXdvl
self.children[iChild].dCcdXdvl[:, iDVLocal] += dCcdXdvl
self.children[iChild].dXrefdXdvl[:, iDVSpanwiseLocal] += dXrefdXdvl
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@joanibal can you confirm that this change is correct? iDVLocal was not defined in this function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks right, this part was copied from the local DV equivalent, but Josh will know better.
We really need a separate batch of tests that verify the correct implementation of all the vars in the child FFDs

ewu63
ewu63 commented Mar 30, 2021
View reviewed changes
pygeo/DVGeometry.py Outdated Show resolved Hide resolved
@ewu63
Copy link
Collaborator Author

ewu63 commented Mar 30, 2021

One thing I'd also like to do is rename numpy imports as np. Would that be okay for everyone?

marcomangano
marcomangano reviewed Mar 31, 2021
View reviewed changes
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.

Very good work!! Some of the things you spotted and fixed reinforce my feeling that there is much more to be addressed with some proper refactoring... but this is great for now

pygeo/geo_utils.py Outdated Show resolved Hide resolved
pygeo/DVConstraints.py Show resolved Hide resolved
pygeo/DVGeometry.py Show resolved Hide resolved
pygeo/DVGeometry.py Show resolved Hide resolved
pygeo/DVGeometryESP.py Show resolved Hide resolved
pygeo/geo_utils.py Show resolved Hide resolved
pygeo/DVConstraints.py Outdated Show resolved Hide resolved
pygeo/DVGeometry.py
@@ -3571,8 +3571,8 @@ def _spanwiselocalDVJacobian(self, config=None):

# TODO: the += here is to allow recursion check this with multiple nesting
# levels
self.children[iChild].dXrefdXdvl[:, iDVLocal] += dXrefdXdvl
self.children[iChild].dCcdXdvl[:, iDVLocal] += dCcdXdvl
self.children[iChild].dXrefdXdvl[:, iDVSpanwiseLocal] += dXrefdXdvl
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks right, this part was copied from the local DV equivalent, but Josh will know better.
We really need a separate batch of tests that verify the correct implementation of all the vars in the child FFDs

pygeo/geo_utils.py Outdated Show resolved Hide resolved
pygeo/DVGeometry.py Outdated Show resolved Hide resolved
@ewu63
Copy link
Collaborator Author

ewu63 commented Mar 31, 2021

Hmm test ./tests/reg_tests/test_DVGeometryVSP.py:RegTestPyGeoVSP_1_parallel_4procs.test_2 seems to fail randomly with a tolerance of 1e-2 instead of 5e-5, not sure what's going on @bbrelje

bbrelje
bbrelje reviewed Apr 1, 2021
View reviewed changes
Copy link
Collaborator

@bbrelje bbrelje left a comment

Choose a reason for hiding this comment

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

@nwu63 that error seems too big (bigger than it had been when the VSP merge was done recently). Does the problem seem to resolve if the ptvecA thing you took out is restored?

It's fair to say there is no upper bound on the possible error due to the known VSP bug when doing parallel finite difference but again, 1e-2 seems way too large.

tests/reg_tests/test_DVGeometryVSP.py Show resolved Hide resolved
bbrelje
bbrelje reviewed Apr 1, 2021
View reviewed changes
Copy link
Collaborator

@bbrelje bbrelje left a comment

Choose a reason for hiding this comment

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

See my comments about the VSP stuff. I didn't read the entire commit (it is huge) but if you followed the described method and all the tests pass, then I am OK with it. It must have been quite a lot of hand-work - thank you for taking care of this.

@ewu63
Copy link
Collaborator Author

ewu63 commented Apr 1, 2021

See my comments about the VSP stuff. I didn't read the entire commit (it is huge) but if you followed the described method and all the tests pass, then I am OK with it. It must have been quite a lot of hand-work - thank you for taking care of this.

If you skip the first big commit that did the formatting, the rest of the commits are relatively easy to follow. If you don't have time for that I also left a bunch of comments highlighting the changes that I think warrant some sort of review, so if you could just take a quick look at those comments that would be great @bbrelje.

@anilyil
Copy link
Contributor

anilyil commented Apr 1, 2021

I reviewed the changes a bit and things look good. Here are my comments:

  1. I tried to reply to all of the comments that I had some understanding of the relevant code. I resolved the stuff that could be resolved, I left a few that is up to you guys on what to do.
  2. The openVSP test being skipped is fine. From what I understand, the simpler spherical test also tests the parallel sensitivities. I am fine with omitting the wing test as long as we test the parallel jacobian at some point. So please confirm that the sphere test does run in parallel, then it is fine for me.
  3. There is one remaining issue with the write wing FFD. The previous indices were bad and while the changes look good, this is the exact type of thing where we can introduce a bug that sends an unexperienced person into a sad rabbit hole. So my suggestion is to check if we already use an FFD file that came out of this script in the tests. If so, we can just compare the script to the file and call it a day.

Besides these items, the PR is good to go for me. I will approve once all changes are done.

@ewu63
Copy link
Collaborator Author

ewu63 commented Apr 1, 2021 •
edited
Loading

  1. I believe there is just one comment left for @joanibal to take a look.
  2. It is being run:
(mpi) ./tests/reg_tests/test_DVGeometryVSP.py:RegTestPyGeoVSP_1_parallel_4procs.test_1 ... OK (00:00:0.13, 339 MB)
  1. See my comment above on this -- we already have a unit test for this function.

bbrelje
bbrelje previously approved these changes Apr 2, 2021
View reviewed changes
anilyil
anilyil previously approved these changes Apr 2, 2021
View reviewed changes
@ewu63 ewu63 dismissed stale reviews from anilyil and bbrelje via 8820eb9 April 2, 2021 17:38
@anilyil anilyil requested review from anilyil and bbrelje April 2, 2021 17:39
@ewu63 ewu63 merged commit 026f579 into master Apr 2, 2021
@ewu63 ewu63 deleted the refactor branch April 2, 2021 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcomangano marcomangano marcomangano left review comments

@anilyil anilyil anilyil approved these changes

@joanibal joanibal Awaiting requested review from joanibal

@bbrelje bbrelje Awaiting requested review from bbrelje

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Formatting
4 participants
@ewu63 @anilyil @bbrelje @marcomangano