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

Make get_cell_range_category() more consistent with get_cell_category() #14015

Merged
merged 1 commit into from Jun 20, 2022

Conversation

peterrum
Copy link
Member

If possible, it would be good to integrate this also into the 9.4 release. The function is newly added in the last months.

Copy link
Member

@kronbichler kronbichler left a comment

Choose a reason for hiding this comment

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

To get this into 9.4, we might need a test to keep track on things.

Comment on lines 1899 to 1901
* maximum catergory of any cell batch. Only in the hp case, it is
* guaranteed that all cells and as a consquence all cell batches have
* the same categroy.
Copy link
Member

Choose a reason for hiding this comment

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

There are a few typos, plus maybe something that can be clarified

Suggested change
* maximum catergory of any cell batch. Only in the hp case, it is
* guaranteed that all cells and as a consquence all cell batches have
* the same categroy.
* maximum category of any cell batch. In the hp case, it is
* guaranteed that all cells and as a consequence all cell batches in a range have
* the same category. Otherwise, there may be different categories in different
* cell batches.

@@ -1912,6 +1917,11 @@ class MatrixFree : public Subscriptor
* run between the given values in the field
* AdditionalData::cell_vectorization_category for non-hp-DoFHandler types
* and return the active FE index in the hp-adaptive case.
*
* @note In the non-hp-DoFHandler case, a categroy of a cell batch is given
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @note In the non-hp-DoFHandler case, a categroy of a cell batch is given
* @note In the non-hp case, a category of a cell batch is given

* @note In the non-hp-DoFHandler case, a categroy of a cell batch is given
* as the maximum category of any of its cell. In the hp case or the case that
* MatrixFree::AdditionalData::cell_vectorization_categories_strict was
* enabled, it is guaranteed that all cells have the same categroy.
Copy link
Member

Choose a reason for hiding this comment

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

You should run a spellchecker on this particular word. 😉

Suggested change
* enabled, it is guaranteed that all cells have the same categroy.
* enabled, it is guaranteed that all cells have the same category.

@peterrum peterrum force-pushed the get_cell_range_category_max branch from bcd2903 to f2d0e65 Compare June 19, 2022 19:19
* AdditionalData::cell_vectorization_category for non-hp-DoFHandler types
* AdditionalData::cell_vectorization_category for the non-hp case
Copy link
Member Author

Choose a reason for hiding this comment

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

@kronbichler There have been other places with references to non-hp-DoFHandler ;)

@peterrum
Copy link
Member Author

peterrum commented Jun 19, 2022

@kronbichler I have added a test! It is a modified version of the test in #14013.

{
const auto category = cell_vectorization_category
[matrix_free.get_cell_iterator(cell, v)->active_cell_index()];
std::cout << category << " ";
Copy link
Member

Choose a reason for hiding this comment

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

Is this output on purpose? It won't be compared, and one will only see it when manually running the test (which might be ok).

Copy link
Member Author

Choose a reason for hiding this comment

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

I would keep it this way. I introduced these for visualization purposes. It looks like this:

0 0 0 0 0 0 0 1 
1 1 1 1 1 1 2 2 
2 2 2 2 2 3 3 3 
3 3 3 3 4 4 4 4 
4 4 4 5 5 5 5 5 
5 5 6 6 6 6 6 6 
6 7 7 7 7 7 7 7 
8 8 8 8 8 8 8 9 

0 0 0 0 0 0 0 
1 1 1 1 1 1 1 
2 2 2 2 2 2 2 
3 3 3 3 3 3 3 
4 4 4 4 4 4 4 
5 5 5 5 5 5 5 
6 6 6 6 6 6 6 
7 7 7 7 7 7 7 
8 8 8 8 8 8 8 
9 

0 0 0 0 0 0 0 

1 1 1 1 1 1 1 

2 2 2 2 2 2 2 

3 3 3 3 3 3 3 

4 4 4 4 4 4 4 

5 5 5 5 5 5 5 

6 6 6 6 6 6 6 

7 7 7 7 7 7 7 

8 8 8 8 8 8 8 

9 

In the test, I am only checking the properties, since outputting such an output would require that we create an output for each vectorization type.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that we should not print this with vectorization. Maybe add a short comment in the code that explains why we have the cout in the test, to guide the reader of test on how to use it?

@peterrum peterrum force-pushed the get_cell_range_category_max branch from f2d0e65 to 291768b Compare June 20, 2022 06:19
Comment on lines +88 to +94
// note: here and below we are using std::cout to output
// information that is useful for debugging; however, since the
// output depends on the hardware and might change if we
// internally modify the way we loop over cells, we don't write
// the data to the log but instead only check that the
// properties that are described in the documentation are
// fulfilled
Copy link
Member Author

Choose a reason for hiding this comment

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

@kronbichler What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Excellent.

@kronbichler kronbichler merged commit 82e0353 into dealii:master Jun 20, 2022
tamiko added a commit that referenced this pull request Jun 21, 2022
…akeover

[9.4] Take over #14015: Make get_cell_range_category() more consistent with get_cell_category()
mkghadban pushed a commit to OpenFCST/dealii that referenced this pull request Sep 8, 2022
…_max

Make get_cell_range_category() more consistent with get_cell_category()
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

2 participants