Updates mfem dependency to [email protected] #1751
Conversation
kennyweiss
added
bug
kennyweiss
force-pushed
the
feature/kweiss/update-mfem
branch
from
e622d30 to
d4110da
Compare
kennyweiss
requested review from
Arlie-Capps,
BradWhitlock,
bmhan12,
gunney1,
jcs15c,
nselliott,
publixsubfan,
rhornung67 and
white238
| //! \brief Accessor for the size of the quadrature rule | ||
| AXOM_HOST_DEVICE | ||
| int getNumPoints() const { return m_nodes.size(); } | ||
| int getNumPoints() const { return static_cast<int>(m_nodes.size()); } |
There was a problem hiding this comment.
I fixed some type conversion warnings from msvc along the way.
(There are many more)
There was a problem hiding this comment.
Note: Some mfem-based shaping tests were failing after the upgrade to [email protected].
Some users were also experiencing memory-related issues w/ shaping against newer versions of mfem.
I think these fixes should resolve the user problem and work against [email protected] as well.
Mfem file type is *.mesh and we should add the background material that we are initializing
Calling mfem::QuadratureFunction::GetValues(int, mfem::DenseMatrix&) on the "positions" quadrature function can invalidate/corrupt the buffer. Use mfem::Reshape() instead. Note: This was tested against [email protected] and [email protected].
kennyweiss
force-pushed
the
feature/kweiss/update-mfem
branch
from
b6586a1 to
3fef819
Compare
| ## Note that NUM_OMP_THREADS delegates to the corresponding argument | ||
| ## in blt_add_test() and sets the OpenMP environment variable OMP_NUM_THREADS. | ||
| ## When AXOM_ENABLE_OPENMP is set and NUM_OMP_THREADS is not provided, | ||
| ## this macros also sets the environment variable OMP_NUM_THREADS=1. |
There was a problem hiding this comment.
This update is for a request in #1749
Tag: @Arlie-Capps @rhornung67
There was a problem hiding this comment.
Thanks, @kennyweiss . I'd suggest to even add words here like
##
## Watch out for confusion between the names of macro argument
## OMP_NUM_THREADS and environment variable NUM_OMP_THREADS.
## These names are close, and related, but not the same.
Maybe that's beating a dead horse, but I can just see future @Arlie-Capps writing a bug because of hubris and hasty assumptions. Documentation gives future me a fighting chance.
There was a problem hiding this comment.
Thanks @Arlie-Capps -- since axom_add_test() and blt_add_test() both have explicit logic to set the OMP_NUM_THREADS environment variable, and this has not been an issue in the past, it seems reasonable (to me) to stick with the current docs update and perhaps make it more explicit in the future if this trips anyone up.
|
|
||
| #if defined(AXOM_USE_MFEM) | ||
| #include "mfem.hpp" | ||
| #include "mfem/linalg/dtensor.hpp" |
There was a problem hiding this comment.
Is this not included in the above header? Should this be reported to mfem?
There was a problem hiding this comment.
It's not:
- https://github.com/mfem/mfem/blob/8e3a7b90fc1934c33b66baafee10fabd1eca0dea/mfem.hpp#L46
- https://github.com/mfem/mfem/blob/master/linalg/linalg.hpp
- https://github.com/mfem/mfem/blob/master/linalg/dtensor.hpp
@v-dobrev, @tzanio -- is dtensor.hpp intentionally left out of mfem.hpp / linalg.hpp ?
Summary
mfem::Reshape()instead ofQuadratureFunction::GetValues()(which was failing w/ [email protected]).TODO: