Restructuring of CVariable into a Contiguous Data Structure #753

pcarruscag · 2019-08-05T16:22:08Z

Proposed Changes

This is the beginning of the work proposed in #716 .
Comments will follow as work progresses.
I would appreciate if we could keep the restructuring of files that use CVariable heavily for when this is done.

Related Work

#716

PR Checklist

Put an X by all that apply. You can fill this out after submitting the PR. If you have any questions, don't hesitate to ask! We want to help. These are a guide for you to know what the reviewers will be looking for in your contribution.

I am submitting my contribution to the develop branch.
My contribution generates no new compiler warnings (try with the '-Wall -Wextra -Wno-unused-parameter -Wno-empty-body' compiler flags).
My contribution is commented and consistent with SU2 style.
I have added a test case that demonstrates my contribution, if necessary.

pcarruscag · 2019-08-05T16:39:50Z

Common/include/toolboxes/C2DContainer.hpp

@@ -0,0 +1,370 @@
+#pragma once


This is the container class I am planning to use to replace the data members of CVariable.
Where CVariable has a scalar it will have a column vector, and where it has an array it will have a row-major matrix (so it can still return the solution as a pointer to the beginning of a row), where it has a matrix we'll see... this supports nesting but at the moment that is only efficient if the inner type has a static size.

Following some of the comments in #716, it supports aligned memory allocation (c++11 required) and compile-time sizes (no dynamic allocation), which may eventually be used to create an "easily-vectorizable" type (e.g. vector of 4 doubles) to explore vectorization of the numerics.

I am hoping to eventually build some basic dense linear algebra capabilities to collect some of the functionality we have throughout the code. I am not a meta-programming expert though, so the interface for that might not be the most user friendly.

I will add the SU2 header to this file and fix the formatting soonish.

Looks very good. It would also be nice to store Nodes of CEdge class in such a container.

Thanks @MicK7, CEdge does not have a lot of overhead since it does not have virtual members, but it does have some... I'll get there at some point.

…ix, matrix->mat of ptr

…su2code/SU2 into feature_contiguous_cvariable_PR

oleburghardt · 2019-10-12T18:17:35Z

SU2_CFD/src/variables/CDiscAdjVariable.cpp

-      Solution_BGS[iVar]          = 0.0;
-    }
-
+    Solution_BGS.resize(nPoint,nVar) = su2double(0.0);


I fixed 3 uninitialized warnings, maybe those are the ones you are referring to?

Yes, and this one has been the only critical one ;)

I also needed Solution_Old

Ok.. it should have had an influence on the residual computation only (it's fixed in the various fixes branch), as it's not used for the solution algorithm. Anyway, might suffice for a crash :p

…uted)

pcarruscag

@economon, I noticed we were allocating Gradient (of the conservatives) and RMatrix for variable classes that do not need those fields, namely IncEuler, Euler, FEA, and DiscAdj.
I moved the allocation down to the derived constructors that do require them, and then noticed that the gradients of flow solver conservatives were still being accessed in the two places pointed below, should I delete the calls I commented out along with the associated routines?

pcarruscag · 2019-10-12T19:18:11Z

SU2_CFD/src/integration_structure.cpp

@@ -311,8 +311,9 @@ void CIntegration::Adjoint_Setup(CGeometry ****geometry, CSolver *****solver_con
      if (iMGLevel != config[iZone]->GetnMGLevels()) {
        SetRestricted_Solution(RUNTIME_FLOW_SYS, solver_container[iZone][INST_0][iMGLevel][FLOW_SOL], solver_container[iZone][INST_0][iMGLevel+1][FLOW_SOL],
                               geometry[iZone][INST_0][iMGLevel], geometry[iZone][INST_0][iMGLevel+1], config[iZone]);
-        SetRestricted_Gradient(RUNTIME_FLOW_SYS, solver_container[iZone][INST_0][iMGLevel][FLOW_SOL], solver_container[iZone][INST_0][iMGLevel+1][FLOW_SOL],
-                               geometry[iZone][INST_0][iMGLevel], geometry[iZone][INST_0][iMGLevel+1], config[iZone]);
+//        ToDo: The flow solvers do not use the conservative variable gradients


Looks like this is outdated. If anything, it should be a restriction of the primitive gradient to make sure everything is well set in multigrid before the adjoint iteration.

I think it is worth trying to remove and see if the continuous adjoint cases fail in the regressions. If so, we can look into it further, since it might need to be adjusted.

On the coarse grids the primitive gradients are never computed, they are only used for viscous residuals but they are zero at that point.
The continuous adjoint cases all pass without calling SetRestricted_Gradient

pcarruscag · 2019-10-12T19:18:22Z

SU2_CFD/src/solver_direct_transition.cpp

@@ -335,8 +335,9 @@ void CTransLMSolver::Viscous_Residual(CGeometry *geometry, CSolver **solver_cont
    numerics->SetTransVar(nodes->GetSolution(iPoint), nodes->GetSolution(jPoint));
    numerics->SetTransVarGradient(nodes->GetGradient(iPoint), nodes->GetGradient(jPoint));

-    numerics->SetConsVarGradient(solver_container[FLOW_SOL]->GetNodes()->GetGradient(iPoint),
-                                 solver_container[FLOW_SOL]->GetNodes()->GetGradient(jPoint));
+//    ToDo: The flow solvers do not use the conservative variable gradients


This one is probably a good question for @vdweide, since he has been updating the transition model in a separate branch. It looks like the viscous numerics is mostly commented out in the develop branch. I expect we should modify this to use the primitive gradients.

No need to worry about the transition model at the moment. Since the data structures have changed so much lately, it is pretty much impossible to carry out a merge between the transition model branch and the develop branch. The implementation must be redone. @economon, we agreed that we would wait with that until PR 777 is merged in. I assume this is still the case.

Sounds good - thanks for confirming @vdweide. Agreed we'll work on this after #777

oleburghardt · 2019-10-14T10:27:05Z

@pcarruscag Any other changes you want to add here? Otherwise I'll merge it.

pcarruscag · 2019-10-14T10:33:40Z

Let's merge it then so the FSI case starts passing, @economon let me know if you want me to remove any of those unused areas and I will push that to #798.

economon · 2019-10-14T15:34:25Z

@pcarruscag : I think we are good to remove those two old gradient sections in #798. I will be working to merge this PR into #790 (and finish up there) next

economon · 2019-10-14T21:33:02Z

Now that I am updating #790 to work with the new structure from this PR (#753), I am seeing that the aligned_alloc() function is not well defined on mac, so xcode/llvm builds are not working on mac at the moment. There are some issue related to C vs C++ with that (depends on the standard you choose for C or C++, 11 or 17, apparently). Might be some ways around it from reading comments by others on the issue: marian-nmt/marian-dev#227.

@pcarruscag : can you give this a quick look when you have some time? Thanks!

add 2D container class

47c35d3

pcarruscag commented Aug 5, 2019

View reviewed changes

pcarruscag added 13 commits August 5, 2019 19:24

replace the types of member of CVariable scalar->vector, vector->matr…

a602e83

…ix, matrix->mat of ptr

update CVariable (base) interface

ed7fd26

turb. variables updated, initializing ctors commented out

c2ac9af

initializers for size variables

4317324

CIncEulerVariable

68ab4d1

CIncNSVariable

f047a6f

CEulerVariable

00f6dc1

CNSVariable

3913d46

HeatFVM, Baseline, and AdjTurb

8b423c8

CFEAVariable

15dfd97

CFEABoundVariable

6218b83

CDiscAdjVariable

a6e08ac

CDiscAdjFEAVariable

ef4c35c

economon mentioned this pull request Aug 7, 2019

Optimize data structures for performance without losing readability/flexibility. #716

Closed

pcarruscag added 14 commits August 7, 2019 12:09

CAdjEuler/NS

11d105d

CSolver and CBaselineSolver compile

8bde588

Direct turbulent solvers compile

43e4eb1

Transition solver compiles

b99fa29

incompressible solvers compile

5095d79

compressible solvers compile

f97e7a0

heat solver compiles

32d8139

FEA solver compiles

561dee9

adjoint turb solver compiles

85a5207

solver adjoint mean

bfa56d3

solver adjoint elasticity

8f52223

solver adjoint discrete, ALL solvers compile

b98e93a

set isVertex boolean of FEA variables

2df4d76

integration structure

0d054d1

cleanup

98de1a1

pcarruscag mentioned this pull request Oct 12, 2019

airfoil_fsi_rbf failing travis #801

Closed

pcarruscag added 6 commits October 12, 2019 11:35

fix un-init variable

7bf2865

Merge branch 'feature_contiguous_cvariable_PR' of https://github.com/…

bc9f391

…su2code/SU2 into feature_contiguous_cvariable_PR

some white spaces [ci skip]

7ca759e

more un-init

92142e9

fix a warning

8beb0ae

allocation of solution gradient and Rmatrix moved to derived ctors

87cf5f4

oleburghardt reviewed Oct 12, 2019

View reviewed changes

comment uses of conservative variable gradients (which are never comp…

f971819

…uted)

pcarruscag commented Oct 12, 2019

View reviewed changes

talbring approved these changes Oct 14, 2019

View reviewed changes

pcarruscag merged commit efaf499 into develop Oct 14, 2019

pcarruscag deleted the feature_contiguous_cvariable_PR branch October 14, 2019 10:33

pr-triage bot added PR: merged PR: unreviewed and removed PR: unreviewed PR: merged labels Oct 14, 2019

rsanfer mentioned this pull request Oct 15, 2019

Re-enable Discrete Adjoint FSI Test Cases #803

Merged

4 tasks

pcarruscag mentioned this pull request Oct 15, 2019

Various fixes #798

Merged

3 tasks

talbring added the changelog:chore label Nov 6, 2019

talbring changed the title ~~Contiguous CVariable~~ Restructuring of CVariable into a Contiguous Data Structure Nov 8, 2019

pr-triage bot added PR: merged and removed PR: unreviewed labels Nov 8, 2019

This was referenced Apr 28, 2020

Contiguous storage of CEdge(s) #959

Merged

Contiguous storage of CPoint(s) #966

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructuring of CVariable into a Contiguous Data Structure #753

Restructuring of CVariable into a Contiguous Data Structure #753

pcarruscag commented Aug 5, 2019 •

edited

Loading

pcarruscag Aug 5, 2019

MicK7 Aug 5, 2019

pcarruscag Aug 7, 2019

oleburghardt Oct 12, 2019

pcarruscag Oct 12, 2019

oleburghardt Oct 12, 2019

pcarruscag left a comment

pcarruscag Oct 12, 2019

economon Oct 13, 2019

pcarruscag Oct 13, 2019

pcarruscag Oct 12, 2019

economon Oct 13, 2019

vdweide Oct 13, 2019

economon Oct 14, 2019

oleburghardt commented Oct 14, 2019

pcarruscag commented Oct 14, 2019

economon commented Oct 14, 2019

economon commented Oct 14, 2019

Restructuring of CVariable into a Contiguous Data Structure #753

Restructuring of CVariable into a Contiguous Data Structure #753

Conversation

pcarruscag commented Aug 5, 2019 • edited Loading

Proposed Changes

Related Work

PR Checklist

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

pcarruscag 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

oleburghardt commented Oct 14, 2019

pcarruscag commented Oct 14, 2019

economon commented Oct 14, 2019

economon commented Oct 14, 2019

pcarruscag commented Aug 5, 2019 •

edited

Loading