Shape by connection #51

kejacobson · 2021-03-04T01:37:29Z

Update components to shape by connection.
Rename TACS's function output to func_struct to avoid name clash with structural force vector (f_struct).
Rename unit_test directory to integration_tests. We need to write some actually unit tests later, but these current tests cover way more code than a unit test.
Changed tacs test to use debug.bdf to run faster and use complex step to be more accurate.
Fix the check scripts in the integration_tests directory.
Removed some unused variables in adflow and rlt components.

mphys/mphys_dvgeo.py

anilyil · 2021-03-07T18:06:58Z

Sorry for hijacking this PR. I pushed a few commits:

The first few commits cleaned up the unused src_indices stuff that we wont need anymore with this PR.
Specifying shape_by_conn with the geometry DVs (which are connected to IVCs) had the wrong outcome. I reverted to specifying the shapes for these inputs and now the code works fine. I will look into this, as I would have expected openmdao to just copy whatever size it had on the local ivc. Instead, it tried doing sth like an even distribution of the vector...
I specified the units for aoa DVs added in adflow tests. This cleared the warnings from that. The VLM solver version I have does not specify units for the aoa variable internally when adding that input. I think you can also remove these errors if you define the unit @bretstanford in the vlm discipline. I only saw aoa so far. I think it is better if we specify the units explicitly both in the components and in the IVCs because adflow uses degrees by default and the vlm solver uses radians.
The adflow derivative test broke with this PR due to shape_by_conn on the adflow coordinate input. The test itself originally specified a non-flattened array for the coordinates, then mphys was using this shape to set the size of the coordinate input array. Interestingly, this worked for fwd analysis but the linearization failed. I flattened the coordinate array in the test script which fixed the issue. Please verify @joanibal
The test_derivatives test in integration_tests/test_tacs_derivs.py fail for me right now. I tried re-copying the input files but I might have messed it up. Can you verify that this test does work with this PR @kejacobson ?
Finally, I think this is a good point to consider how we would do parallel multipoint in the multipoint.py code. It looks like the shape_by_connection approach will work for all connections made inside the scenarios. For "serial multipoint", i.e. cases that run back to back on the full comm, we also do not need to specify the src_indices for connections from mp-level to the scenario level. For "parallel multipoint", i.e. the comm is split into say half, and the two scenarios run in parallel, we will need to specify the src_indices explicitly for the connections between the multipoint level and the scenario levels. Thankfully, the only default connection here are for the mesh surface coordinates, whose size is readily available from the builders. I will bring this up in the call and we can discuss the implementation further.

kejacobson · 2021-03-07T19:00:12Z

No worries about hijacking the PR.

3. I specified the units for aoa DVs added in adflow tests. This cleared the warnings from that. The VLM solver version I have does not specify units for the aoa variable internally when adding that input. I think you can also remove these errors if you define the unit @bretstanford in the vlm discipline. I only saw aoa so far. I think it is better if we specify the units explicitly both in the components and in the IVCs because adflow uses degrees by default and the vlm solver uses radians.

Yes, I've already updated our version of VLM to use 'aoa' with units='rad'.

5. The `test_derivatives` test in `integration_tests/test_tacs_derivs.py` fail for me right now. I tried re-copying the input files but I might have messed it up. Can you verify that this test does work with this PR @kejacobson ?

I just checked it again, and it passes for me. How is it failing for you? Are you using a complex-mode compiled version of TACS when you run it?

6. Finally, I think this is a good point to consider how we would do parallel multipoint in the `multipoint.py` code. It looks like the `shape_by_connection` approach will work for all connections made inside the scenarios. For "serial multipoint", i.e. cases that run back to back on the full comm, we also do not need to specify the src_indices for connections from mp-level to the scenario level. For "parallel multipoint", i.e. the comm is split into say half, and the two scenarios run in parallel, we will need to specify the src_indices explicitly for the connections between the multipoint level and the scenario levels. Thankfully, the only default connection here are for the mesh surface coordinates, whose size is readily available from the builders. I will bring this up in the call and we can discuss the implementation further.

I've got a branch with a refactor of various pieces that I'll discuss in the call tomorrow. One of the changes is a ParallelGroup version of Multipoint.

anilyil · 2021-03-07T19:06:20Z

Regarding point 5, if it works for you, then its fine for me. I did not use a complex-compiled tacs and just ran it with whatever I had w/o reading the code. I think the failure was a value error (expected value of 1 and got 0 or the other way around).

Regarding point 6, sure, we can discuss further.

joanibal

Looks good, I have just a few questions/suggestions

joanibal · 2021-03-10T17:04:30Z

mphys/geo_disp.py

@@ -13,19 +13,11 @@ def initialize(self):
    def setup(self):
        nnodes = self.options['number_of_surface_nodes']


Can this be removed now that size information is given by the inputs and outputs

I think that may be possible. Currently the pattern everywhere is outputs are sized and inputs are shape_by_conn. Another option would be to make the pattern that solvers set sizes of both inputs and outputs and transfer schemes are shape_by_conn. For the second way that may get less straightforward when you have multiple components between solvers like struct -> disp_xfer -> geo_disp -> aero. Either disp_xfer or geo_disp has to set the size of u_aero which means one of them needs a number_of_nodes like option.

But x_aero0 is set by the mesh outside of the solver group.
So, I believe you could you copy_shape to size the output x_aero or input u_aero without the need for a number_of_nodes like option.

I have created issue #55 to track unresolved comments like this

joanibal · 2021-03-10T17:10:39Z

mphys/mphys_dvgeo.py

-        ind2 = (self.comm.rank + 1) * nVal
-
-        self.add_input(dvName, shape=nVal, src_indices=np.arange(ind1, ind2, dtype=int))
+        self.add_input(dvName, shape=nVal)


For local shape variables, I think it makes sense to set the initial value to zeros.

My concern is that if an this input is used with an independent variable comp the shape vars will all start at 1.0 which corresponds to a 100% increase from the baseline.
Although, I'm not very familiar with how independent variable comps work internally.

actually thats a good point. I think in general we can default to returning the initial values from this add DV call, then the user can add it to the ivc with the correct values. Or maybe the set input defaults is what we need to use here?

In the monday telecon, I brought up that that there may be an OpenMDAO bug related to this conversation. I had set the val of an input to zeros and OM connected an autoIVC which set the value to ones anyways. I haven't had time to verify that that is in fact what was happening or recreate it in a test small enough to submit as an issue to OM. Just wanted to bring that up so that you know I plan to ask them to fix it if that is actually what was happening.

I created issue #54 to keep track of this.

I have created issue #54 to track the discussion here

joanibal · 2021-03-10T17:14:01Z

mphys/mphys_meld.py

@@ -33,39 +33,18 @@ def setup(self):

        #self.set_check_partial_options(wrt='*',method='cs',directional=True)

-        struct_ndof = self.struct_ndof


Can these be determined are run time now instead of passing size information as options?

I have created issue #55 to track unresolved comments like this

joanibal · 2021-03-10T17:17:51Z

tests/integration_tests/test_tacs_derivs.py


        for var, err in data.items():

            rel_err = err['rel error']  # ,  'rel error']
-            assert_near_equal(rel_err.forward, 0.0, 1e-2)
-        data = self.prob.check_totals(of=['mp.s0.struct_funcs.funcs.f_struct'], wrt='f_struct', step=3e-5, step_calc='rel')  # out_stream=None
+            assert_near_equal(rel_err.forward, 0.0, 1e-8)


The error should be a lot smaller than 1e-8, right?
Or is the larger tolerance related to known issues about the conditioning of shell elements?

I am not going to create a new issue for this because it sounds like a more general discussion not directly relevant for this PR. Please follow up @joanibal

anilyil · 2021-03-15T14:40:38Z

I created a PR to the OpenMDAO repo related to the issue I saw here with shape_by_conn: OpenMDAO/OpenMDAO#1957 Once we fix the issues and the tests pass, we can also get back to this PR.

joanibal · 2021-03-16T20:00:22Z

mphys/mphys_meld.py

-        self.add_output('u_aero', shape = aero_nnodes*3,
-                               val=np.zeros(aero_nnodes*3),
-                               desc='aerodynamic surface displacements')
+        self.add_output('u_aero', shape = self.aero_nnodes*3,


Couldn't we use shape_by_conn here too?

Actually we have been looking into the shape_by_conn stuff with @JustinSGray today and I think we are most likely going to disallow an "upstream" shape by connection. So that is, the output getting it's shape from the input its connected to. We will explain more, I dont think its certain now but there is a corner case in parallel runs that is pretty nasty and will just complicate things. We also need to fix a bug on the openmdao side, but thats a separate case.

So the convention we can use for mphys: all outputs are explicitly sized and inputs can be shape by conn.

I think that convention is overly restrictive.
Couldn't we copy the shape of x_aero0 to size the output?

We will explain more, I dont think its certain now but there is a corner case in parallel runs that is pretty nasty and will just complicate things.

Can you add an issue on the OpenMDAO repo so others can consider the issue and the solution too.

Yeah, I will work on the test and the issue today. I will post the new PR here.

I have created issue #55 to track unresolved comments like this

anilyil · 2021-03-16T20:30:34Z

As I mentioned in a reply above, I suggest we don't make any changes to this PR until we decide if we will change the shape_by_conn behavior on the openmdao side. The PR as is will work because we only use the "downstream" mode of propagating the shape info; outputs are sized and inputs inherit their shape. The "upstream" mode of propagating the shape info creates an ambiguity when an output of a serial component is connected to a parallel component. @JustinSGray is planning to disallow this mode of shape propagation to prevent cases like this. I will update my tests and explanation in OpenMDAO/OpenMDAO#1957 after our discussion today, and that should clarify things. I think its better to wait for that to be resolved before we merge this PR.

anilyil · 2021-03-17T10:12:31Z

I have created the new PR related to the issues I was referring to at: OpenMDAO/OpenMDAO#1964

kejacobson added 16 commits February 28, 2021 20:23

Switch to shape_by_conn

f2754a0

Right location of f_struct

92fc4fc

Switch bdf in tacs unit test back

f6066fd

Change tacs function name from f_struct to func_struct

15c7eb7

Fix tacs_check_partials

64351fd

Consistent name of test files

50efc52

Change name for usage of func_struct in examples and tests

3f30615

CS for test_tacs_derivs.py

cbb9d54

More test fixes

054d6f4

Add executable attribute to shell script to wget test files

a7bb7cb

Set all IVs into vlm for check derivatives scripts

5e7cbab

Rename test directory

9de9b30

Point tacs_check_partials to input_files dir

af8e77a

Merge branch 'master' into shape_by_conn

ad96ac3

Switch wingbox mesh to debug mesh to run tacs test faster

68d4490

Tighten TACS test tolerances

cb4de98

kejacobson requested review from joanibal and anilyil March 4, 2021 01:37

Merge branch 'master' into shape_by_conn

140eddc

anilyil reviewed Mar 7, 2021

View reviewed changes

mphys/mphys_dvgeo.py Outdated Show resolved Hide resolved

anilyil added 5 commits March 7, 2021 11:04

clened up the unnecessary indices in geo_disp

ff1146f

fixed input addition indices

95f0447

removed unused variable in adflow wrapper

776e52c

added units to aoa in adflow examples to remove warnings

703b790

fixed adflow deriv test

4e00793

joanibal requested changes Mar 10, 2021

View reviewed changes

joanibal reviewed Mar 16, 2021

View reviewed changes

anilyil mentioned this pull request Mar 17, 2021

Test all combinations of dynamic variable sizing in parallel runs, take 2 OpenMDAO/OpenMDAO#1964

Closed

kejacobson mentioned this pull request Mar 17, 2021

Refactor hierarchy #53

Merged

This was referenced Mar 21, 2021

pyGeo input addition and the default values #54

Closed

extend the usage of shape_by_conn #55

Closed

anilyil merged commit f0e5c69 into OpenMDAO:master Mar 21, 2021

anilyil mentioned this pull request Mar 22, 2021

shape_by_conn is broken for parallel to serial connections #56

Closed

kejacobson deleted the shape_by_conn branch March 31, 2021 16:58

anilyil mentioned this pull request Mar 13, 2022

MPhys wrapper: Modify initial values of DVGeo inputs in OpenMDAO mdolab/pygeo#121

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shape by connection #51

Shape by connection #51

kejacobson commented Mar 4, 2021

anilyil commented Mar 7, 2021

kejacobson commented Mar 7, 2021

anilyil commented Mar 7, 2021

joanibal left a comment

joanibal Mar 10, 2021

kejacobson Mar 11, 2021

joanibal Mar 16, 2021

anilyil Mar 21, 2021

joanibal Mar 10, 2021

anilyil Mar 10, 2021

kejacobson Mar 11, 2021

anilyil Mar 21, 2021

anilyil Mar 21, 2021

joanibal Mar 10, 2021

anilyil Mar 21, 2021

joanibal Mar 10, 2021

anilyil Mar 21, 2021

anilyil commented Mar 15, 2021

joanibal Mar 16, 2021 •

edited

Loading

anilyil Mar 16, 2021

joanibal Mar 16, 2021

anilyil Mar 17, 2021

anilyil Mar 21, 2021

anilyil commented Mar 16, 2021

anilyil commented Mar 17, 2021

		@@ -13,19 +13,11 @@ def initialize(self):
		def setup(self):
		nnodes = self.options['number_of_surface_nodes']

		@@ -33,39 +33,18 @@ def setup(self):

		#self.set_check_partial_options(wrt='*',method='cs',directional=True)

		struct_ndof = self.struct_ndof

Shape by connection #51

Shape by connection #51

Conversation

kejacobson commented Mar 4, 2021

anilyil commented Mar 7, 2021

kejacobson commented Mar 7, 2021

anilyil commented Mar 7, 2021

joanibal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anilyil commented Mar 15, 2021

joanibal Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anilyil commented Mar 16, 2021

anilyil commented Mar 17, 2021

joanibal Mar 16, 2021 •

edited

Loading