Generalize statistics collection code for MeshClipper #1754
Conversation
Counter statistics are now placed in a conduit::Node so new statistics can be added without modifying the mesh clipper implementation. The mesh clipper can just add and globally reduce and log statistics without having to know what quantities they are. The screen level parameter is also added. It's a run-time parameter for studying how effective screening is. It's integrated with statistics so it's included on this branch.
gunney1
requested review from
Arlie-Capps,
BradWhitlock,
cyrush,
jcs15c,
kennyweiss and
rhornung67
kennyweiss
left a comment
kennyweiss
left a comment
There was a problem hiding this comment.
Looks like a useful feature @gunney1
Please check that this doesn't have a performance overhead in cases where you don't want/need to collect statistics.
| * @param [in] shapeMesh Blueprint mesh to shape into. | ||
| * @param [out] ovlap Shape overlap volume of each cell | ||
| * in the \c shapeMesh. It's initialized to zeros. | ||
| * @param [out] statistics Optional statistics to record | ||
| * consisting of child nodes with integer values. | ||
| * | ||
| * The default implementation has no specialized method, | ||
| * so it's a no-op and returns false. | ||
| * | ||
| * If this method returns false, then exactly one of the | ||
| * @c getShapesAs...() methods must be provided. | ||
| * | ||
| * @return True if clipping was done and false if a no-op. | ||
| * | ||
| * This method need not be implemented if labelCellsInOut() | ||
| * returns true. | ||
| * | ||
| * Setting the statistics is not required except for getting | ||
| * accurate statistics. | ||
| * | ||
| * If implementation returns true, it should ensure these | ||
| * post-conditions hold: | ||
| * @post ovlap.size() == shapeMesh.getCellCount() | ||
| * @post ovlap.getAllocatorID() == shapeMesh.getAllocatorId() | ||
| */ | ||
| virtual bool specializedClipCells(quest::experimental::ShapeMesh& shapeMesh, | ||
| axom::ArrayView<double> ovlap) | ||
| axom::ArrayView<double> ovlap, | ||
| conduit::Node& statistics) | ||
| { | ||
| AXOM_UNUSED_VAR(shapeMesh); | ||
| AXOM_UNUSED_VAR(ovlap); | ||
| AXOM_UNUSED_VAR(statistics); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
- @param [out] statistics Optional statistics to record
consisting of child nodes with integer values.
Since statistics is given by reference, rather than a pointer which could be nullptr (or perhaps a std::optional), it looks like it is required, rather than optional.
Is there a performance cost to passing in the statistics? E.g. if the statistics are written to the conduit node within an inner loop, and then ignored.
There was a problem hiding this comment.
I don't access the Node in inner loops. That doesn't work anyway when the inner loop is a GPU kernel. A typical use is to use a RAJA sum object in the inner loop then get the value after the loop. I have not seen any significant cost to computing statistics this way.
| /*! | ||
| * @brief Log clipping statistics. | ||
| * Intended for developer use. | ||
| * | ||
| * This is a collective method if MPI-parallel. | ||
| */ | ||
| void logClippingStats(bool local = false, bool sum = true, bool max = false) const; |
There was a problem hiding this comment.
Please document these three parameters
| /*! | ||
| * @brief Set the level of screening, | ||
| * intended for developer use. | ||
| */ | ||
| void setScreenLevel(int screenLevel) { m_screenLevel = screenLevel; } |
There was a problem hiding this comment.
Please document the valid levels and their meanings
| /*! | ||
| * @brief Get global assorted clipping statistics, | ||
| * intended for developer use. | ||
| * | ||
| * This is a collective method if MPI-parallel. | ||
| */ | ||
| conduit::Node getGlobalClippingStats() const; |
There was a problem hiding this comment.
Gut-check: Is this supposed to return by value?
There was a problem hiding this comment.
Yes. The global clipping stats are not stored internally.
| } | ||
|
|
||
| m_impl->collectOnIndices(tetLabels.view(), tetsOnBdry); | ||
| m_impl->remapTetIndices(cellsOnBdry, tetsOnBdry); |
There was a problem hiding this comment.
Gut check: Was this change intentional? Or perhaps a bug fix? I don't see an API change in this PR for Impl::remapTetIndices()
There was a problem hiding this comment.
It fixes an error I made when I split up the huge PR into smaller ones. I didn't catch the error immediately because this code wasn't exercised until the PRs I submitted later. So this is a correction to a code that wasn't being exercised, but I thought it made sense to keep the correction as close as possible to the place where the error was made. In the original huge PR, it was correct.
Reduce device copies when collecting indices
…LNL/axom into feature/gunney/mesh-clipper-statistics
Summary
Statistics are stored in the key-value pairs of a
conduit::Node. New quantities can be added without modifying the mesh clipper implementation. The mesh clipper can just add, globally reduce and log statistics without having to know what quantities they are.The screen level parameter is also added. It's a run-time parameter for studying how effective screening is. It's integrated with statistics so it's included on this branch.
This change is part of the clipping performance work and has been separated to make smaller change sets. Some of the statistics code introduced here may seem unused, but they will be used in up-coming PRs.