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

Triangulated surface constraint using geograd and multiple DVGeos per DVCon #52

Merged
merged 33 commits into from
Feb 4, 2021

Conversation

bbrelje
Copy link
Collaborator

@bbrelje bbrelje commented Oct 30, 2020

Purpose

This PR adds the KS-based triangulated surface constraint from the Brelje et al AIAAJ paper

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

Reg tests with good coverage have been added. All previous DVConstraints tests pass after the refactor.

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

@bbrelje bbrelje requested a review from a team as a code owner October 30, 2020 22:47
joanibal
joanibal previously approved these changes Nov 13, 2020
joanibal
joanibal previously approved these changes Jan 4, 2021
@anilyil
Copy link
Contributor

anilyil commented Jan 18, 2021

This looks good in general. One discussion; you are currently allowing multiple DVGeos kicking around with a single DVCon. I was doing this in a slightly different way with DVGeoMulti, where I had an upper-level dvgeo object, that kept track of all of the individual DVgeos for each component. that way we can also define intersections etc. I think we can come up with a common way to handle multiple dvgeos and use a consistent approach.

Here is what I recommend: I understand you don't want to make changes at this point. I also do not want to work on geometry stuff. BUT, I think we can come up with a much better scheme if we sit down together and do a mini hackathon with you. I am guessing we can take care of this work within a few hours on a call.

What do you say @bbrelje? If you don't want to make changes, that is totally fine by me and the PR is good as far as I am concerned.

@anilyil
Copy link
Contributor

anilyil commented Jan 18, 2021

I talked to ben and we decided to merge this PR now, and we will refactor these stuff for harmony when we merge the dvgeomulti code. We estimate to do this in April, when ben will be done running the cases for his thesis work. I will create an issue about this and also move my dvgeomulti code to my public fork because the paper is accepted and in press now.

anilyil
anilyil previously approved these changes Jan 18, 2021
@anilyil
Copy link
Contributor

anilyil commented Jan 18, 2021

I have created the issue that details the refactoring in #57.

@bbrelje
Copy link
Collaborator Author

bbrelje commented Jan 23, 2021

I forgot I never included the triangulated volume constraint when I ported from the tensorflow-based geograd to the fortran one. I will need to deprecate that function or add it to geograd this weekend (probably the former). Block merge for now.

@bbrelje bbrelje changed the title Triangulated surface constraint using geograd and multiple DVGeos per DVCon WIP: Triangulated surface constraint using geograd and multiple DVGeos per DVCon Jan 23, 2021
@anilyil anilyil marked this pull request as draft January 23, 2021 09:01
@anilyil
Copy link
Contributor

anilyil commented Jan 23, 2021

@bbrelje I converted the PR to a draft. I understand you want to work on this more? I did the draft thing to prevent somebody accidentally merging this since both reviewers approved.

@bbrelje bbrelje dismissed stale reviews from anilyil and joanibal via 8b77fb7 January 31, 2021 22:58
@bbrelje bbrelje changed the title WIP: Triangulated surface constraint using geograd and multiple DVGeos per DVCon Triangulated surface constraint using geograd and multiple DVGeos per DVCon Feb 1, 2021
@bbrelje bbrelje marked this pull request as ready for review February 1, 2021 01:32
@ewu63
Copy link
Collaborator

ewu63 commented Feb 4, 2021

geograd tests failed previously, this has now been fixed. I've verified locally that all tests pass, so I'll merge manually.

@ewu63 ewu63 merged commit 82fd55a into mdolab:master Feb 4, 2021
bernardopacini pushed a commit to bernardopacini/pygeo that referenced this pull request Mar 17, 2021
… DVCon (mdolab#52)

Co-authored-by: Benjamin Brelje <tg856804@login3.stampede2.tacc.utexas.edu>
Co-authored-by: Anil Yildirim <42349285+anilyil@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.

4 participants