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

Added coordinate transformations and a few fixes #157

Merged
merged 20 commits into from
Oct 5, 2022

Conversation

anilyil
Copy link
Contributor

@anilyil anilyil commented Sep 16, 2022

Purpose

This PR adds the capability of doing coordinate transformations with pygeo. Specifically, we can now have the adflow geometry sit on a different reference frame but have a different reference frame for the parameterization. For example, the FFD can stay roughly aligned with the x-streamwise y-spanwise z-lift directions, whereas the CFD geometry is rotated and displaced. This is achieved by a callback function that the user provides that applies the transformations. Only differences in dvgeo is that we intercept the main add pointset, update, and derivative routines to apply the transformation as the first or the last thing that is done. Derivatives are also transferred with the same approach; we just include all of the rotations but dont apply the transformations. The same method can be implemented in other DVGeo classes as well.

Besides this, the PR has some minor bug fixes. Specifically, defining lower and upper bounds for sectional DVs were broken. This is fixed now. There is also a small fix for using pygeo with the symmetry option, but this may not be fully operational.

Expected time until merged

1-2 weeks

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

I know this is an early draft, but I am confused about what goes into self.coord_xfer. It looks like it has a very specific function signature.
I am also not familiar with FFD configurations that would benefit from the symmetry option, but we can discuss that at a later stage.

@anilyil
Copy link
Contributor Author

anilyil commented Sep 16, 2022

@marcomangano, you are correct that this is a draft, but I just added the explanation for that functionality: https://github.com/mdolab/pygeo/pull/157/files#diff-6f6a800de0054468f520f9d63ccbf34c246bc5cc490311587b7068ec8c27fb85R650-R708 Let me know if it's clear or not. I will also try to add a test for this.

@anilyil anilyil marked this pull request as ready for review September 16, 2022 23:29
@anilyil anilyil requested a review from a team as a code owner September 16, 2022 23:29
@anilyil
Copy link
Contributor Author

anilyil commented Sep 16, 2022

I am also thinking of adding the testing required for mdolab/pyspline#47 in here. Is that okay? It looked like pyspline itself does not have many tests, plus, I would rather test the full embedding with DVGeo in the loop since thats how people interact with those routine 99% of the time.

@codecov
Copy link

codecov bot commented Sep 16, 2022

Codecov Report

Merging #157 (7949637) into main (4641625) will decrease coverage by 10.52%.
The diff coverage is 31.91%.

@@             Coverage Diff             @@
##             main     #157       +/-   ##
===========================================
- Coverage   63.75%   53.22%   -10.53%     
===========================================
  Files          47       47               
  Lines       11786    11819       +33     
===========================================
- Hits         7514     6291     -1223     
- Misses       4272     5528     +1256     
Impacted Files Coverage Δ
pygeo/pyBlock.py 45.37% <ø> (-1.65%) ⬇️
pygeo/parameterization/DVGeo.py 65.29% <30.43%> (-0.38%) ⬇️
pygeo/parameterization/designVars.py 77.65% <100.00%> (ø)
pygeo/parameterization/DVGeoMulti.py 0.39% <0.00%> (-89.38%) ⬇️
pygeo/constraints/areaConstraint.py 50.50% <0.00%> (-25.42%) ⬇️
pygeo/constraints/DVCon.py 68.04% <0.00%> (-3.69%) ⬇️
pygeo/topology.py 54.92% <0.00%> (-0.23%) ⬇️
pygeo/geo_utils/node_edge_face.py 45.71% <0.00%> (+2.44%) ⬆️
... and 2 more

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

…olab#47 is merged. Updated the input file archive to include sabet's fuselage FFD
@anilyil
Copy link
Contributor Author

anilyil commented Sep 24, 2022

I added the tests from @sseraj's fuselage case. Some points are extremely challenging to converge, but I dont test everything. The set thats actively tested fails w/o the solver fixes in mdolab/pyspline#47 and passes with these fixes.

The previous solver also had a termination criteria preventing the most problematic points from working. Now they do work, but you need a ridiculously large number of iterations. I only tested a few like this, but I am disabling them because that FFD is unreasonably skewed.

This is ready for review.

…aptop with 1e-15 rel tolerance. the error is at 1.1e-15. Moved the tolerance to 1e-14, which should be plenty accurate
@sseraj
Copy link
Contributor

sseraj commented Sep 27, 2022

To dos:

  • Create an issue to remove embTol
  • Clean up points for the tests
  • Create an issue for symmetry plane bugs
  • Fix coord_xfer docstring

@anilyil
Copy link
Contributor Author

anilyil commented Sep 27, 2022

All TODOs addressed. This one is ready for review.

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.

Good stuff, see my suggestions for minor changes below

pygeo/parameterization/DVGeo.py Outdated Show resolved Hide resolved
pygeo/parameterization/DVGeo.py Show resolved Hide resolved
pygeo/parameterization/DVGeo.py Show resolved Hide resolved
pygeo/parameterization/DVGeo.py Show resolved Hide resolved
pygeo/parameterization/DVGeo.py Show resolved Hide resolved
pygeo/parameterization/DVGeo.py Outdated Show resolved Hide resolved
pygeo/parameterization/designVars.py Show resolved Hide resolved
tests/reg_tests/test_DVGeometry.py Show resolved Hide resolved
pygeo/parameterization/DVGeo.py Outdated Show resolved Hide resolved
pygeo/parameterization/DVGeo.py Show resolved Hide resolved
tests/reg_tests/test_DVGeometry.py Outdated Show resolved Hide resolved
@anilyil
Copy link
Contributor Author

anilyil commented Sep 30, 2022

this is also ready for round 2. thanks again for all the feedback

Copy link
Contributor

@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.

Looks good! We can merge this once the pySpline changes are on the Docker images and we rerun the CI.

@marcomangano marcomangano merged commit 022e6b2 into mdolab:main Oct 5, 2022
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