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

Add coupled elemental solution capability #9758

Merged
merged 4 commits into from Sep 7, 2017

Conversation

YaqiWang
Copy link
Contributor

This is an enhancement of #9690 for coupling local DoFs. It is needed by my special kernels which has pre-assembled mass matrix.

@dschwen
Copy link
Member

dschwen commented Aug 30, 2017

We can couple values of elemental variables with coupledValue, so the name might be a bit confusing (maybe everyone else is much smarter though...). Are you trying to get the solution vector entries for the DOFs on an element with a higher order monomial (discontinuous) shape function?

@andrsd
Copy link
Contributor

andrsd commented Aug 30, 2017

@dschwen His methods are named coupledElementalSlnXYZ - there is no value in the name - so there should not be a confusion...

@moosebuild
Copy link
Contributor

moosebuild commented Aug 30, 2017

Job Documentation on 2615b91 wanted to post the following:

View the site here

@YaqiWang
Copy link
Contributor Author

@dschwen Your second part is right, but not the first part. @andrsd answered you about it. DoFs in the solution vector are typically not the value on quadrature points.

@YaqiWang
Copy link
Contributor Author

Call for review.

@permcody
Copy link
Member

permcody commented Aug 31, 2017

This really expands a core interface in MOOSE. If you add to Coupleable, you add to just about everything so this needs to be correct. Does this interface make sense for AuxKernels, BCs, Materials, ScalarKernels, ElementalUserObjects, NodalUserObjects, SideUserObjects, Indicators, etc?

I agree with @dschwen, the name, although somewhat consistent with MOOSE internals will be exposed to users and it isn't very clear what it is? When do I use coupleValue versus coupledSln? Also Sln is terrible for a user interface.

Tagging @idaholab/moose-team

@andrsd
Copy link
Contributor

andrsd commented Aug 31, 2017

@YaqiWang I think you will need to add a check that if _nodal == false you throw an error. This flag is true when we are in nodal systems like nodal BCs, nodal auxkernels, nodal UOs, etc. So if the API is called from there, people should get an error, because there is no concept of a current element.

@permcody This can be used in any system of elemental-type, kernels, BCs, element UO, side UO. He is pulling out the solution coefficients from an element. Based on my understand from our conversation last week, he is trying to do his own numerical quadrature. Similar to what we do with nodal values for the porous flow guys. What I assume is that he has the need for 2 types quadrature rules at the same time, i.e. some parts of his system are integrated by the usual q-rule we provide, but some needs to be integrated by some other rule. So this would be the easiest way how to work around our "single q-rule rules them all" limitation we have in MOOSE.

@YaqiWang
Copy link
Contributor Author

@andrsd Not true, even the variable is nodal, we can still get the local DoFs on an element for the variable, just like we can couple nodal variables in an elemental aux kernel. Like with LAGRANGE, we can break the variable into an elemental variable with L2_LAGRANGE.

The second part is almost right. I have two parts, one part uses q-rule, the other part cannot use q-rule directly but with a sophisticated transform and ended up with a 1D q-rule. Most importantly, I need to do these integration in the setup stage and store the assembled matrices, otherwise on-the-fly evaluation would be too costly.

@andrsd
Copy link
Contributor

andrsd commented Aug 31, 2017

@YaqiWang

Not true, even the variable is nodal, we can still get the local DoFs on an element for the variable, just like we can couple nodal variables in an elemental aux kernel.

You are right, but that's not what I am saying. If we are at a node (like in a Nodal BC), there is no concept of an element. We iterate over nodes, not elements in these systems. MooseVariable::computeElemValues() is not called in these loops, so you would be providing memory that has some garbage in it. The _nodal flag does not mean a nodal variable, but a nodal system, i.e. a system that iterates over nodes, not elements.

@permcody
Copy link
Member

My problem here is preventing the user from calling the wrong coupling methods to get values. Calling coupledValueSln() will work just fine, but it won't necessarily give you the value you might expect. This will be directly exposed to users.

@YaqiWang
Copy link
Contributor Author

YaqiWang commented Aug 31, 2017

I see, where is this _nodal, in Assembly? If so, we cannot add the check in coupledElementalSln(), because it returns a reference. However, we can add checks in MooseVariable::computeElemValues(). @permcody I am not too concerned with that. We had the similar situation anyway, like _q_points, _normals, etc.

@YaqiWang
Copy link
Contributor Author

I choose the returned type DenseVector purposely because the chance of using them with DenseVector interface functions is probably higher than just accessing them with [] operator.

@andrsd
Copy link
Contributor

andrsd commented Aug 31, 2017

I see, where is this _nodal, in Assembly?

It is passed into Coupleable's c-tor from the system that requires coupleable properties, it is a member variable. We already use it for checking in other coupledXYZ methods.

@YaqiWang
Copy link
Contributor Author

Oh, I was not pay attention, thought it is nodal variable or not. You are right, I should protect these functions with if (!_nodal).

Copy link
Member

@permcody permcody left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation for this new method isn't very helpful. I'm not saying the old documentation is any better but now that we have so many interfaces, we really need to make this better. Take a look:

// Returns value of a coupled variable
coupledValue(...)

// Returns local DoFs of a coupled variable
coupledElementalSln(...)

So they both return something about a coupled variable? Why local DoFs? What does that mean? Do I care? Also what is "Sln"? How do I index into this thing?

Yes, some of these questions are definitely our fault. We don't have detailed instructions on how to use the existing interfaces either, but this is really adding fuel to the fire. I'm not against adding these, but I think the names should be changed, and I think the description of this new public API should be clearly laid out.

@YaqiWang
Copy link
Contributor Author

YaqiWang commented Sep 5, 2017

Yes, they both for coupled variables. Local DoFs means the elements in the solution vector relevant with the local elements, which is different from the variable values on the quadrature points in coupledValue. Do you have a better name in mind for this? The size of it is usually referred as the number of degrees of freedom (DoF). For example, MONOMIAL in 3d would be having (p+1)*(p+2)*(p_3)/6, where p is the order of the variable. Sln is in short of Solution. This is needed by me for directly manipulating the DoFs.

@permcody
Copy link
Member

permcody commented Sep 5, 2017

@YaqiWang - That's great! Please put that kind of description into the doxygen.

I'm open for suggestions, but I think we should spell out the name of any word we use for the external interface. "Sln" is fine for internal use, but let's be smart about the API.

@YaqiWang
Copy link
Contributor Author

YaqiWang commented Sep 5, 2017

Weird, I did not see your comments and about to complain the turn around time... I am afraid that the name will be too long. How about coupledSolutionDoFs?

@permcody
Copy link
Member

permcody commented Sep 5, 2017

That's not bad. @andrsd, any thoughts on better name?

@andrsd
Copy link
Contributor

andrsd commented Sep 5, 2017

coupledSolutionDoFs is good.

@YaqiWang
Copy link
Contributor Author

YaqiWang commented Sep 5, 2017

Will make the change shortly.

@YaqiWang
Copy link
Contributor Author

YaqiWang commented Sep 5, 2017

Any more comments?

permcody
permcody previously approved these changes Sep 6, 2017
Copy link
Member

@permcody permcody left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok to me, @andrsd - any other comments?

@andrsd
Copy link
Contributor

andrsd commented Sep 6, 2017

The testing can be improved to also test coupledSolutionDoFsOld and coupledSolutionDoFsOlder. The ElementMomentSum postprocessor can take a parameter to use either current, old or older version of the coupling API. You can reuse the test file and pass this parameter via a command line parameters and have 3 different tests - one for each. The idea is used in CoupledScalarAux if you need more details on implementation.

@YaqiWang
Copy link
Contributor Author

YaqiWang commented Sep 6, 2017

I am also going to make another postprocessor to get neighbor solutions. Will update the PR soon.

@YaqiWang
Copy link
Contributor Author

YaqiWang commented Sep 6, 2017

This is ready.

@permcody
Copy link
Member

permcody commented Sep 6, 2017

Did you see David's suggestion? He wants you to test the old and older APIs as well. Gives you a suggestion and tells you which test to use as template.

@YaqiWang
Copy link
Contributor Author

YaqiWang commented Sep 6, 2017

Yes, I took an easier approach, adding a new parameter use_old, which combines with implicit will test the old and older APIs.

@YaqiWang
Copy link
Contributor Author

YaqiWang commented Sep 7, 2017

When will this be merged? Or more comments?

@permcody permcody merged commit 7a5cf8f into idaholab:devel Sep 7, 2017
@andrsd
Copy link
Contributor

andrsd commented Sep 8, 2017

@YaqiWang Your easier approach tested the API in MooseVariable, but did not test Coupleable::coupledSolutionDoFsOlder. Which my suggestion was targeting...

@YaqiWang
Copy link
Contributor Author

Right, my approach actually tested the if statement in Old. I guess this is minor and we can leave it as it is unless you have strong opinions.

YaqiWang added a commit to YaqiWang/moose that referenced this pull request Sep 11, 2017
YaqiWang added a commit to YaqiWang/moose that referenced this pull request Sep 11, 2017
permcody added a commit that referenced this pull request Sep 12, 2017
Fix a valgrind unitialized-value error introduced in PR #9758
jarons pushed a commit to jarons/moose that referenced this pull request Oct 5, 2017
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

5 participants