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

Array kernel #13528

Merged
merged 69 commits into from Jul 10, 2019
Merged

Array kernel #13528

merged 69 commits into from Jul 10, 2019

Conversation

YaqiWang
Copy link
Contributor

Close #6881. Superseding #13314 and #13459.

@YaqiWang
Copy link
Contributor Author

@lindsayad This is just rebasing and make Function pointer const in ArrayFunctionIC.

@YaqiWang
Copy link
Contributor Author

I also moved the description of array kernels in newsletter from June to July.

@moosebuild
Copy link
Contributor

moosebuild commented Jun 10, 2019

Job Documentation on 0bc4aee wanted to post the following:

View the site here

This comment will be updated on new commits.

@YaqiWang
Copy link
Contributor Author

YaqiWang commented Jun 10, 2019

https://mooseframework.inl.gov/docs/PRs/13528/site/source/variables/ArrayMooseVariable.html is the main link to the documentation. Unfortunately I cannot figure out where the added newsletter in MOOSE Docs is due to the issue I just created in #13529. Tag @friedmud

@YaqiWang
Copy link
Contributor Author

The failure in Pronghorn demonstrated the dangerousness of using global params. All the failing tests in Pronghorn has a global parameter component that set to 0. Now with this PR, component is a parameter for AddVariableAction, which has default value of 1. The global param accidentally changed the default value!

@YaqiWang
Copy link
Contributor Author

YaqiWang commented Jun 19, 2019

I did some tests and verified the speed-up of using array kernels with Rattlesnake. Attached are the results with a simple 2D setup with 1089 nodes:
array-kernel
array-kernel2
The first plot shows the grind times, defined as the total cpu time in residual evaluations divided by number of evaluations and the number of DoFs. The second plot shows the ratio of grind times. Memory is also saved, with 480 variables, the memory usage is decreased from about 0.7G to 0.4G. We clearly see the benefit of using array kernels!

@lindsayad
Copy link
Member

lindsayad commented Jun 19, 2019 via email

@YaqiWang
Copy link
Contributor Author

Added a test for array kernel save-in and did some optimizations revealed necessary for the rattlesnake test.

@moosebuild
Copy link
Contributor

Job Precheck on bfb7306 wanted to post the following:

Your code requires style changes.

A patch was auto generated and copied here
You can directly apply the patch by running, in the top level of your repository:

curl -s https://mooseframework.inl.gov/docs/PRs/13528/style.patch | git apply -v

Alternatively, with your repository up to date and in the top level of your repository:

git clang-format f9fcbae7d9328e2093debe6004ea7a33698706d9

@YaqiWang
Copy link
Contributor Author

When can this be reviewed?

@lindsayad
Copy link
Member

Looks like a rebase is needed. If that's cleaned up, I will merge unless @friedmud still wants to review

@YaqiWang
Copy link
Contributor Author

YaqiWang commented Jul 5, 2019

Thanks @lindsayad I do not know why I did not notice your reply!

@YaqiWang
Copy link
Contributor Author

@lindsayad I think we can merge this now. Then I can start fixing apps.

@lindsayad lindsayad merged commit e9917bb into idaholab:next Jul 10, 2019
@YaqiWang
Copy link
Contributor Author

The failing apps are: Pronghorn, Bighorn, R7, Moltres, etc. Unfortunately, I can only access Pronghorn and Moltres, will need to have somebody in MOOSE team for others.

@lindsayad
Copy link
Member

lindsayad commented Jul 10, 2019 via email

@YaqiWang
Copy link
Contributor Author

The fix should be fairly quick if I can sit on the side. I believe the failures are mostly caused by the changes in AddVariableAction. Sorry @lindsayad This has been here too long, I forgot something.

@lindsayad
Copy link
Member

lindsayad commented Jul 11, 2019 via email

@YaqiWang
Copy link
Contributor Author

I am wrong, I can fix R7, thm. ;-)

@lindsayad
Copy link
Member

lindsayad commented Jul 11, 2019 via email

@YaqiWang
Copy link
Contributor Author

Oh, I am . working on thm too, lol. I will let you do it then.

@YaqiWang
Copy link
Contributor Author

Thanks man. Have a good sleep!

@lindsayad
Copy link
Member

lindsayad commented Jul 11, 2019 via email

/// Holds the solution at current quadrature points
const ArrayVariableValue & _u;
/// The component
const unsigned int _component;
Copy link
Member

Choose a reason for hiding this comment

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

this would have been better named as _components or _num_componentsand the corresponding input parameter as well. We uses component to denote the index of a single component (I believe that is what caused the pronghorn failure - not the danger of global parameters, but an unfortunately named parameter in this PR).

@lindsayad
Copy link
Member

lindsayad commented Jul 11, 2019 via email

@YaqiWang
Copy link
Contributor Author

YaqiWang commented Jul 11, 2019

Possibly. When I add this parameter, I kind of simplified the word to make it essential, like family, order, component, but maybe components is better. Or if you can come up other even better names. BTW, order to scalar variables is actually component to stand variables.

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.

ArrayKernel and ArrayBoundaryCondition
6 participants