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

Improve MultiMat Sphinx documentation #1476

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from

Conversation

BradWhitlock
Copy link
Member

This PR adds Sphinx documentation for Axom's MultiMat component. A couple of missing methods were also added that help the user query field properties.

Resolves #1362.

@bmhan12
Copy link
Contributor

bmhan12 commented Dec 9, 2024

FYI, Here's the link to the generated ReadTheDocs for this PR: https://axom.readthedocs.io/en/feature-whitlock-multimat_documentation/axom/multimat/docs/sphinx/index.html

Comment on lines 76 to 78
The CMR determines how materials are allocated to each cell but it does not determine
how much of each material is present in the cell. Volume fractions determine how much of each material
exists in each cell and is given as a percentage. If a cell contains materials A and B

Choose a reason for hiding this comment

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

Please consider rephrasing to "The CMR determines which materials are present in each cell; volume fractions determine how much of each material is present in each cell." This shortens the first two sentences to one sentence with two parallel phrases: to me, this helps with clarity. Also, I don't think you need "given as a percentage," since the next sentence gives an example.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed.

Comment on lines 80 to 81
cell are: *0.2* and *0.8*. Note that the sum of volume fractions in a cell must equal 1
to account for all of the cell. Volume fractions must be provided for every valid

Choose a reason for hiding this comment

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

Will multimat itself object if the volume fractions don't sum to 1? Is there a consistency check for this error mode, or will it just wait until it bites your algorithm?

Copy link
Member Author

@BradWhitlock BradWhitlock Dec 12, 2024

Choose a reason for hiding this comment

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

It doesn't enforce it really, but it will flag the error if MultiMat::isValid() is called. I added a statement to this effect.

Comment on lines 92 to 95
called a dense field. Dense fields are easy to understand since they have values for
every cell/material pair and they take more memory since zeroes must be provided even
for invalid cell/material pairs. Fields can also be sparse, which eliminates the zeroes
where a material does not exist.

Choose a reason for hiding this comment

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

Suggestion: rephrase to "Dense fields are easy to understand: they have values for every cell/material pair. Dense fields store zeros for cells where a material is not present. Fields can also be sparse, saving memory by eliminating the zeros where a material does not exist."

(I think both "zeros" and "zeroes" are correct. Not sure on that.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed.

Comment on lines 38 to 52
.. code-block:: cpp

constexpr int nComponents = 1;
double data[] = {0.,
1.,
2.,
// more values ...
};
axom::ArrayView<double> dataAV(data, sizeof(data) / sizeof(double));
mm.addField("x2",
axom::multimat::FieldMapping::PER_CELL,
axom::multimat::DataLayout::CELL_DOM,
axom::multimat::SparsityLayout::SPARSE,
dataAV,
nComponents);

Choose a reason for hiding this comment

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

It would be very helpful to me if you could expand this example to have a diagram of (say) a 2x3 mesh with maybe two materials. Perhaps you could skip the call to setCellMatRel() and setVolfracField(). But it would help if you had three calls to addField(), one with PER_CELL, one with PER_MAT, and one with PER_CELL_MAT and CELL_DOM. These could all be DENSE, because you haven't gotten to the discussion of index sets yet. I think seeing everything fully written out would help readers learn these concepts.

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 moved code snippets into a basic.cpp example that compiles and added more examples of adding fields.

Comment on lines 255 to 256
For algorithms where sparse data traversal is desired, there are multiple options. The
MultiMat indexing sets can be used directly. Cells are iterated first in this example
Copy link

@Arlie-Capps Arlie-Capps Dec 10, 2024

Choose a reason for hiding this comment

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

Suggest condensing these two sentences to something like "For algorithms where sparse data traversal is desired, the MultiMat indexing sets can be used directly as an alternative to dense traversal patterns." I dunno. Maybe it's just a style question.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed.

Copy link

@Arlie-Capps Arlie-Capps left a comment

Choose a reason for hiding this comment

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

Thank you, Brad! This is a great addition to our documentation.

API Documentation
-----------------

Doxygen generated API documentation can be found here: `API documentation <../../../../doxygen/html/coretop.html>`_
Copy link
Contributor

Choose a reason for hiding this comment

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

This links to the top of core instead of multimat. Maybe the last bit needs to be multimattop.html?
Doxygen multimat page: https://axom.readthedocs.io/en/feature-whitlock-multimat_documentation/doxygen/html/multimattop.html

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks - fixed it.

+--------------------+----------------------------------------------------------+
| FieldMapping | Meaning |
+====================+==========================================================+
| PER_CELL | The field contains up to ncells*ncomponents values (for |
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: A space before and after the * in this table and elsewhere may make the sizing dimensions more clear.
(ncells * ncomponents versus ncells*ncomponents).

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 added spaces.

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.

Improve "Multimat" documentation
3 participants