-
Notifications
You must be signed in to change notification settings - Fork 55
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
Add spanwise shape variables #75
Conversation
I following the same process that Nick used for |
I don't understand DVGeometry's inner working as well as @marcomangano so I will pass this down to him. The figures in the PR does a great job at explaining what it does, is there a way we can include them in the docs as well? |
@joanibal I will carefully review this tomorrow! Just a few points for now:
I see the point of having this extra feature but I am trying to figure out how usable it can be for general cases. |
You can do both. Selecting a "single rows" is a bit tricky, but you would do the same way you would for any set of local shape variables (use pointSelect and/or volList) |
Just one last thought before I dive into the code: if not in place already, I would add an |
I am not sure where this discrepancy comes from. Sometimes I've had no problems training stuff locally, other times things behave in really strange ways. In this case though, it appears that the ref file was not added to the PR branch? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of new code - but luckily pretty close to the previous GeoDVSectionLocal
implementation. As this is an entirely new feature, I don't see risks of breaking anything (unless the user mixes vars in a weird way?) so I think this is pretty much ready - see the comments on testing though.
I added a bunch of comments, mostly for me to try and better understand how pyGeo
works. Hope to turn this PR into a chance to improve the code current documentation, any concise comment or explanation is welcome.
Also, I completely agree that at some point pyGeo
will need some major refactoring
@@ -144,9 +145,11 @@ def __init__(self, fileName, complex=False, child=False, faceFreeze=None, *args, | |||
self.nDVG_T = None | |||
self.nDVL_T = None | |||
self.nDVSL_T = None | |||
self.nDVSW_T = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you had to come up with a different name because SL
was taken, right?
Maybe we could add some line comments here to explain every acronym, just to make things a tiny easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the new comments in code to address this
@@ -1955,7 +2122,7 @@ def computeTotalJacobianCS(self, ptSetName, config=None): | |||
return | |||
|
|||
def addVariablesPyOpt(self, optProb, globalVars=True, localVars=True, | |||
sectionlocalVars=True, ignoreVars=None, freezeVars=None): | |||
sectionlocalVars=True, spanwiselocalVars=True, ignoreVars=None, freezeVars=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add documentation for sectionlocalVars
and spanwiselocalVars
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
N = self.FFD.embededVolumes['child%d_coef'%(iChild)].N | ||
self.children[iChild].dCcdXdvl = numpy.zeros((N*3, self.nDV_T)) | ||
|
||
iDVSpanwiseLocal = self.nDVSW_count |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see most of this comes from _sectionlocalDVJacobian
but it would be great to have some more inline comments, I am struggling a bit to follow the flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see new added comments
leList = [[0, 0, 0 ], [-radius/2, 0, height]] | ||
xAxis = [-1, 0, 0] | ||
yAxis = [0, 1, 0] | ||
DVCon.addLERadiusConstraints(leList, nSpan=5, axis=yAxis, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this comes from the previous test, but I am not familiar with this script. Why are we using it?
chordDir=xAxis, scaled=False) | ||
# DVCon.writeTecplot('cylinder_constraints.dat') | ||
|
||
funcs = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks we are only testing the DVCon sens here... which I guess it is pretty much the same than testing the DV sens directly? See my comment before the review about an additional test - if needed
L M A O, good catch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I apologize for the delay in review. I am going to focus on the testing because I don't totally understand the implementation code and I trust you did it generally correctly.
Is there any way of verifying with the cylinder test as written that the feature is working as intended, and not just that it's producing the answer you expect. I am not sure a LERadiusConstraint alone is enough for that. a 2d grid of thickness constraints might be (would want to verify that the numbers are the same in the spanwise direction
There also ought to be a verification that this DV isn't used on a non-2D problem; or the test coverage needs to be extended to cover 3D cases.
I added another test using a 2d grid of thickness constraints as Ben suggested. It is ready for review again. |
Not sure why but there are some merge conflicts, could you resolve those so the newly added test gets run on Azure? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for the comments! Let's wait for the tests to run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are good to go! (see comment below). @bbrelje we need your approval of the changes before we merge this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think it would be best to test the situation where someone tries to use this functionality for a 3D problem (in my view it should throw an error and the test should assert that it throws an error). One minor cleanup item (remove the extra debug statement from DVConstraints).
@bbrelje maybe I'm not getting what you mean by 3D problem. In the newest test, these design variables are used for a 3D problem. I agree the main application is 2D, but it isn't wrong to use these design variables for 3D. |
My understanding here is that these DVs can be used in 3D cases, and there are tests/docs covering usage. I'm going to merge this but if you think this PR can be improved, please make an issue @bbrelje. |
Purpose
These new design variables control all the FDD nodes along a line in a chosen spanwise direction
This means that for 2D shape optimization with ADflow, you can use spanwise local design variables to parametrize the 2D shape without the need for added linear constraints.
I think this is a lot easier to explain with an examples
Consider the following rectangular FFD with it i,j, and k axes as shown in the figure
If a airfoil surface mesh is embedded in the FFD as shown
Then adding spanwise shape variables like this
would add the 6 design variables that correspond to the six colors in this figure. Each design variables would control the z location of two FFD points.
The test case uses the existing cylinder FFD
Type of change
What types of change is it?
Select the appropriate type(s) that describe this PR
Testing
Explain the steps needed to test the new code to verify that it does indeed address the issue and produce the expected behavior.
Checklist
Put an
x
in the boxes that apply.flake8
andblack
to make sure the code adheres to PEP-8 and is consistently formatted^formatting introduced a large amount of change not relevant to the feature
^ I will after feedback