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

MooseVariable needs to be cleaned up #9690

Closed
YaqiWang opened this issue Aug 17, 2017 · 5 comments
Closed

MooseVariable needs to be cleaned up #9690

YaqiWang opened this issue Aug 17, 2017 · 5 comments
Labels
C: Framework P: normal A defect affecting operation with a low possibility of significantly affects. T: task An enhancement to the software.

Comments

@YaqiWang
Copy link
Contributor

YaqiWang commented Aug 17, 2017

Description of the enhancement or error report

Few things I have noticed in the past:

  1. Several _need_xxx are not set correctly, such as nodalSln() not setting _need_nodal_u. This is a subtle bug;
  2. Lots of code duplication in computeXXXValues;
  3. Does not allow skip evaluations on quadrature points in cases that no quantities is needed (refer An idea to make MOOSE applications run 10 times faster #9014). I propose split computeXXXValues into two parts: one is actually computeNodalValues, just copying the dofs out from the solution vectors, another is for evaluating quantities on quadrature points. In this way, early exit will be easily enabled;
  4. Later calls after initial setup to obtain things protected by _need_xxx, such as slnOld() is dangerous and should be disabled. This can be done with mooseError or mooseAssert with the flag provided by FEProblem in those calls;
  5. Some of simple calls are in header file, while some are in source file.

Rationale for the enhancement or information for reproducing the error

Although I really do not want to do this because MooseVariable is at the critical code path, it is time to fix these issues.

Identified impact

(i.e. Internal object changes, limited interface changes, public API change, or a list of specific applications impacted)
Should be none. Fixing item 4 might require application patches.

@YaqiWang
Copy link
Contributor Author

I can work on this if you all agree.

@andrsd
Copy link
Contributor

andrsd commented Aug 17, 2017

A few comments before you start doing anything. MooseVariable works with 3 kinds of values:

  1. values interpolated in q-points (when on an element)
  2. nodal values when on an element
  3. nodal values when on a node (i.e. nodal BC, etc.)

_need_nodal_xyz is related to 2 and it certainly copies values from the solution vector, because it assumes nodal FE. Methods that are slnXYZ are not related to _need_nodal_XYZ. They reference a different memory in MooseVariable and that memory is set in reinitAux, not in computeXYZ.

@permcody permcody added C: Framework P: normal A defect affecting operation with a low possibility of significantly affects. T: task An enhancement to the software. labels Aug 17, 2017
YaqiWang added a commit to YaqiWang/moose that referenced this issue Aug 22, 2017
YaqiWang added a commit to YaqiWang/moose that referenced this issue Aug 22, 2017
YaqiWang added a commit to YaqiWang/moose that referenced this issue Aug 22, 2017
YaqiWang added a commit to YaqiWang/moose that referenced this issue Aug 22, 2017
YaqiWang added a commit to YaqiWang/moose that referenced this issue Aug 23, 2017
@YaqiWang
Copy link
Contributor Author

You did not read my post carefully. The method missing _need_nodal_u is nodalSln() which is returning _nodal_u.

@andrsd
Copy link
Contributor

andrsd commented Aug 23, 2017

nodalSln() is for case 3: nodal objects working at nodes. In these systems, nodal values are computed in a different way then when we are on elements.

nodalValue() is for case 2: nodal values when in elemental objects like kernels, BCs, elem PPS, etc. Here we need _need_nodal_u, because these values are computed on demand (i.e. user has to explicitly ask for their values).

In #9690, you have an elemental object, i.e. case 2, but calling API for case 3. That's why your code fails. Use the right API and all will be fine.

The reason why we do not need _need_XYZ for case 3 is that when we reinit, we flip the defined flag and we compute the nodal value (which is just a vector lookup) just for the defined one. There are no gradients, or interpolation or whatnot, so we do not need this optimization. On the other hand when on elements we do need this, because there is a lot of things that can be computed. So, let's not mix 2 and 3 and let's just call the right API.

YaqiWang added a commit to YaqiWang/moose that referenced this issue Aug 30, 2017
YaqiWang added a commit to YaqiWang/moose that referenced this issue Aug 30, 2017
YaqiWang added a commit to YaqiWang/moose that referenced this issue Aug 30, 2017
YaqiWang added a commit to YaqiWang/moose that referenced this issue Aug 30, 2017
YaqiWang added a commit to YaqiWang/moose that referenced this issue Aug 30, 2017
YaqiWang added a commit to YaqiWang/moose that referenced this issue Aug 31, 2017
YaqiWang added a commit to YaqiWang/moose that referenced this issue Aug 31, 2017
YaqiWang added a commit to YaqiWang/moose that referenced this issue Sep 5, 2017
YaqiWang added a commit to YaqiWang/moose that referenced this issue Sep 5, 2017
YaqiWang added a commit to YaqiWang/moose that referenced this issue Sep 6, 2017
YaqiWang added a commit to YaqiWang/moose that referenced this issue Sep 6, 2017
YaqiWang added a commit to YaqiWang/moose that referenced this issue Sep 6, 2017
YaqiWang added a commit to YaqiWang/moose that referenced this issue Sep 11, 2017
jarons pushed a commit to jarons/moose that referenced this issue Oct 5, 2017
jarons pushed a commit to jarons/moose that referenced this issue Oct 5, 2017
jarons pushed a commit to jarons/moose that referenced this issue Oct 5, 2017
@YaqiWang
Copy link
Contributor Author

YaqiWang commented Feb 7, 2018

This issue is vague and I feel I am done on cleaning up. So closing.

@YaqiWang YaqiWang closed this as completed Feb 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Framework P: normal A defect affecting operation with a low possibility of significantly affects. T: task An enhancement to the software.
Projects
None yet
Development

No branches or pull requests

3 participants