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

Enhance documentation for sum_into_values #16382

Merged
merged 1 commit into from Jan 15, 2024

Conversation

jh66637
Copy link
Contributor

@jh66637 jh66637 commented Dec 24, 2023

By now, FEPointEvaluation::integrate(buffer, flags) zeroed out the whole buffer and possibly wrote to a subset of DoFs. Therefore, the code becomes tedious and error-prone if multiple FEPointEvaluation objects work on the same buffer. This PR aims to zero out only the values the objects actually work on.

fe_eval_first_selected_component_0.integrate(dst,flags); 
fe_eval_first_selected_component_1.integrate(dst,flags); 

While above code works as expected, if we use FEEvaluation objects with FEPointEvaluation currently one would have to write

// Zeroes out everything and writes some values to buffer
fe_point_eval_first_selected_component_0.integrate(buffer,flags); 

// We have to use sum_into_values=true so that the line does not zero out anything.
// Instead we relie on a different object to zero out the values before this line. 
// With sum_into_values=false the values that are written to the buffer in the first line are zeroed out.
fe_point_eval_first_selected_component_1.integrate(buffer,flags,true); 

Additionally, from the documentation of sum_into_values: "Flag specifying if the integrated values should be summed into the solution values. Defaults to false." I would expect that only those DoFs are changed that the object works on.

Maybe there are reasons why everything is currently zeroed out that I couldn't think of. Can you comment on this after the holidays @bergbauer?


EDIT: As described in #16382 (comment) using separate buffers results in similar code as for FEFaceEvaluation. Therefore, I simply enhanced the documentation.

@jh66637 jh66637 changed the title [WIP] Don't zero out complete buffer [WIP] FEPointEvaluation::integrate(): Only zero out relevant DoFs Dec 24, 2023
@jh66637 jh66637 marked this pull request as ready for review December 24, 2023 14:13
@jh66637 jh66637 changed the title [WIP] FEPointEvaluation::integrate(): Only zero out relevant DoFs FEPointEvaluation::integrate(): Only zero out relevant DoFs Dec 24, 2023
@bergbauer
Copy link
Contributor

This behavior existed before I started working on FEPointEvaluation, I have added the option sum_into_values because of efficiency reasons. The zeroing-out is tested by quite some tests, see e.g. https://github.com/dealii/dealii/blob/35f9cb91f997df8e9d9d3405592b49d5fecf0c83/tests/matrix_free/point_evaluation_04.cc

In my opinion, we should make the behavior as similar as possible to FEEvaluation, so I would be in favor of changing it to your suggestion. If we decide to keep the current behavior, we should try to improve the documentation to make this obvious. What do you think @peterrum @kronbichler

With your suggestion, do I still need to zero out before the quadrature loop? The components worked on are written anyway if sum_into_values==false.

@peterrum
Copy link
Member

peterrum commented Jan 2, 2024

In my opinion, we should make the behavior as similar as possible to FEEvaluation, so I would be in favor of changing it to your suggestion.

Yes, I think this is the right step. When reviewing step-89 (from where the above example comes), it was quite non-intuitive that you need to add the second time.

@jh66637
Copy link
Contributor Author

jh66637 commented Jan 3, 2024

With your suggestion, do I still need to zero out before the quadrature loop? The components worked on are written anyway if sum_into_values==false.

We only have to zero out in one corner case (where nothing has to be done). The rest should be fine by simply skipping the lines.

@bergbauer
Copy link
Contributor

We only have to zero out in one corner case (where nothing has to be done). The rest should be fine by simply skipping the lines.

Nice, then we can get rid of the complicated check!

@jh66637
Copy link
Contributor Author

jh66637 commented Jan 3, 2024

@peterrum I can take a shot once this is marked ready to test, so I know which tests are failing.

@peterrum
Copy link
Member

peterrum commented Jan 3, 2024

/rebuild

@jh66637 jh66637 force-pushed the dont_zero_out_whole_buffer branch 2 times, most recently from a3e4a41 to 43a98d5 Compare January 5, 2024 16:34
@jh66637
Copy link
Contributor Author

jh66637 commented Jan 5, 2024

Maybe the behavior was motivated because the buffer is typically used for evaluate() and integrate(). If not all DoFs are zeroed out, there might be DoF values left in the buffer after integrate() which is not intuitive. Using different buffers for each FEPointEvaluation object, it is possible to write the code the same way as with FEEvaluation which also holds different buffers internally. This requires to call distribute_local_to_global() twice, which is also done for FEEvaluation objects. Using one buffer and calling distribute_local_to_global always directly after integrate() works as well.

I gave this some more thoughts and am now in favor of keeping everything as it is. Does this also make sense to you @bergbauer @peterrum? If so, I will simply enhance the documentation.

@bergbauer
Copy link
Contributor

bergbauer commented Jan 8, 2024

Maybe the behavior was motivated because the buffer is typically used for evaluate() and integrate(). If not all DoFs are zeroed out, there might be DoF values left in the buffer after integrate() which is not intuitive. Using different buffers for each FEPointEvaluation object, it is possible to write the code the same way as with FEEvaluation which also holds different buffers internally. This requires to call distribute_local_to_global() twice, which is also done for FEEvaluation objects. Using one buffer and calling distribute_local_to_global always directly after integrate() works as well.

In my opinion, FEPointEvalaution should work on the buffers of FEEvaluation and we should write the code such that this works intuitively.

@jh66637
Copy link
Contributor Author

jh66637 commented Jan 8, 2024

@bergbauer using the buffers from corresponding FEFaceEvaluation objects works already intuitively by zeroing out the whole buffer.

@jh66637 jh66637 force-pushed the dont_zero_out_whole_buffer branch 2 times, most recently from 7bc6e23 to 98646f1 Compare January 8, 2024 17:45
@jh66637 jh66637 changed the title FEPointEvaluation::integrate(): Only zero out relevant DoFs Enhance documentation for sum_into_values Jan 8, 2024
@jh66637
Copy link
Contributor Author

jh66637 commented Jan 8, 2024

@bergbauer @peterrum I clarified the documentation. Using separate buffers (e.g. from corresponding FEEvaluation objects) leads to intuitive code.

@peterrum
Copy link
Member

peterrum commented Jan 8, 2024

Maybe the behavior was motivated because the buffer is typically used for evaluate() and integrate(). If not all DoFs are zeroed out, there might be DoF values left in the buffer after integrate() which is not intuitive. Using different buffers for each FEPointEvaluation object, it is possible to write the code the same way as with FEEvaluation which also holds different buffers internally. This requires to call distribute_local_to_global() twice, which is also done for FEEvaluation objects. Using one buffer and calling distribute_local_to_global always directly after integrate() works as well.

Can you give an example code?

@jh66637
Copy link
Contributor Author

jh66637 commented Jan 9, 2024

Sure. Below you see a pseudocode using different buffers (here using the buffers from FEEvaluation objects:

fe_point_eval_first_selected_component_0.integrate(fe_eval_first_selected_component_0.get_buffer(),flags); 
fe_point_eval_first_selected_component_1.integrate(fe_eval_first_selected_component_1.get_buffer(),flags); 

fe_eval_first_selected_component_0.distribute_local_to_global();
fe_eval_first_selected_component_1.distribute_local_to_global();

@kronbichler kronbichler merged commit ded657d into dealii:master Jan 15, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants