Conversation
|
CDash for AT1 results [Only accessible from Sandia networks] |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request performs extensive cleanup and correction of Doxygen documentation across the Ifpack2 and Tpetra packages, including the removal of obsolete Doxyfile tags and the addition of missing parameter descriptions. Review feedback highlights several remaining inaccuracies in documentation, specifically where 'Global' is used instead of 'Local' for column index descriptions in various filter classes, a misleading parameter name in OverlappingRowGraph, and an outdated brief tag in InverseDiagonalKernel.
|
|
||
| /// \file Ifpack2_Details_DiagonalKernel_decl.hpp | ||
| /// \file Ifpack2_Details_InverseDiagonalKernel_decl.hpp | ||
| /// \brief Declaration of Ifpack2::Details::DiagonalKernel |
There was a problem hiding this comment.
The \brief description still refers to DiagonalKernel, but this file is for InverseDiagonalKernel. Please update it to match the file name and content.
| /// \brief Declaration of Ifpack2::Details::DiagonalKernel | |
| /// \brief Declaration of Ifpack2::Details::InverseDiagonalKernel |
| /// | ||
| /// \param localRow [in] Local index of the row. | ||
| /// \param indices [out] Local column indices in that row that are | ||
| /// \param gblColInds [out] Local column indices in that row that are |
There was a problem hiding this comment.
The parameter name gblColInds is misleading for the getLocalRowCopy method, as the description correctly states it returns local column indices. This appears to be a copy-paste error from the getGlobalRowCopy documentation above. It should likely be lclColInds to match the naming convention used in getLocalRowView (line 243).
| \param LocalRow - (In) Local row number for which indices are desired. | ||
| \param Indices - (Out) Global column indices corresponding to values. | ||
| \param Values - (Out) Row values | ||
| \param indices - (Out) Global column indices corresponding to values. |
There was a problem hiding this comment.
In getLocalRowView, the documentation incorrectly describes the indices parameter as "Global column indices". Since this method provides a view of local indices, the description should be updated to "Local column indices" for accuracy, consistent with the description in getLocalRowCopy (line 174).
| \param indices - (Out) Global column indices corresponding to values. | |
| \param indices - (Out) Local column indices corresponding to values. |
| Note: If \c DropRow does not belong to this node, then \c indices is set to null. | ||
| */ | ||
| \param LocalRow - (In) Local row number for which indices are desired. | ||
| \param indices - (Out) Global column indices corresponding to values. |
There was a problem hiding this comment.
In getLocalRowView, the documentation incorrectly describes the indices parameter as "Global column indices". It should be "Local column indices" to correctly reflect the behavior of this local view method.
| \param indices - (Out) Global column indices corresponding to values. | |
| \param indices - (Out) Local column indices corresponding to values. |
| \param Values - (Out) Row values | ||
| \pre <tt>isGloballyIndexed() == false</tt> | ||
| \post <tt>indices.size() == getNumEntriesInLocalRow(LocalRow)</tt> | ||
| \param indices - (Out) Global column indices corresponding to values. |
There was a problem hiding this comment.
In getLocalRowView, the documentation incorrectly describes the indices parameter as "Global column indices". It should be updated to "Local column indices".
| \param indices - (Out) Global column indices corresponding to values. | |
| \param indices - (Out) Local column indices corresponding to values. |
| \param Values - (Out) Row values | ||
| \pre <tt>isGloballyIndexed() == false</tt> | ||
| \post <tt>indices.size() == getNumEntriesInDropRow(LocalRow)</tt> | ||
| \param indices - (Out) Global column indices corresponding to values. |
There was a problem hiding this comment.
In getLocalRowView, the documentation incorrectly describes the indices parameter as "Global column indices". It should be "Local column indices".
| \param indices - (Out) Global column indices corresponding to values. | |
| \param indices - (Out) Local column indices corresponding to values. |
| \param Values - (Out) Row values | ||
| \pre <tt>isGloballyIndexed() == false</tt> | ||
| \post <tt>indices.size() == getNumEntriesInLocalRow(LocalRow)</tt> | ||
| \param indices - (Out) Global column indices corresponding to values. |
There was a problem hiding this comment.
In getLocalRowView, the documentation incorrectly describes the indices parameter as "Global column indices". It should be "Local column indices".
| \param indices - (Out) Global column indices corresponding to values. | |
| \param indices - (Out) Local column indices corresponding to values. |
| Note: If \c DropRow does not belong to this node, then \c indices is set to null. | ||
| */ | ||
| \param LocalRow - (In) Local row number for which indices are desired. | ||
| \param indices - (Out) Global column indices corresponding to values. |
There was a problem hiding this comment.
In getLocalRowView, the documentation incorrectly describes the indices parameter as "Global column indices". It should be "Local column indices".
| \param indices - (Out) Global column indices corresponding to values. | |
| \param indices - (Out) Local column indices corresponding to values. |
Signed-off-by: Christian Glusa <caglusa@sandia.gov>
csiefer2
force-pushed
the
pr-mirror-15184
branch
from
73ff083 to
d456027
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request focuses on extensive documentation cleanup and synchronization across the Ifpack2 and Tpetra packages. The changes primarily involve updating Doxygen comments to match function signatures, adding missing parameter descriptions (particularly for 'apply' and 'applyMat' methods), and correcting file headers. A specific issue was identified in 'Ifpack2_Details_OverlappingRowGraph_decl.hpp' where the documentation for 'getLocalRowCopy' was updated with an incorrect parameter name, creating a mismatch with the actual code change.
| /// \param numIndices [out] Number of indices returned in \c gblColInds. | ||
| /// | ||
| /// This method throws std::runtime_error if \c indices is not large | ||
| /// This method throws std::runtime_error if \c gblColInds is not large | ||
| /// enough to hold the column indices in row \c localRow. If row | ||
| /// <tt>localRow</tt> does not belong to this process, then | ||
| /// <tt>indices</tt> is not modified and \c numIndices is set to | ||
| /// <tt>gblColInds</tt> is not modified and \c numIndices is set to | ||
| /// Teuchos::OrdinalTraits<size_t>::invalid() on output. | ||
| virtual void | ||
| getLocalRowCopy(local_ordinal_type localRow, | ||
| nonconst_local_inds_host_view_type& gblColInds, | ||
| nonconst_local_inds_host_view_type& indices, |
There was a problem hiding this comment.
In getLocalRowCopy, the parameter name in the function signature was correctly changed from gblColInds to indices to reflect that it handles local indices. However, the Doxygen documentation was incorrectly updated to use gblColInds in several places (lines 213, 215, and 218), which now creates a mismatch with the updated signature. The documentation should be updated to use indices to remain consistent with the code.
| /// \param numIndices [out] Number of indices returned in \c gblColInds. | |
| /// | |
| /// This method throws std::runtime_error if \c indices is not large | |
| /// This method throws std::runtime_error if \c gblColInds is not large | |
| /// enough to hold the column indices in row \c localRow. If row | |
| /// <tt>localRow</tt> does not belong to this process, then | |
| /// <tt>indices</tt> is not modified and \c numIndices is set to | |
| /// <tt>gblColInds</tt> is not modified and \c numIndices is set to | |
| /// Teuchos::OrdinalTraits<size_t>::invalid() on output. | |
| virtual void | |
| getLocalRowCopy(local_ordinal_type localRow, | |
| nonconst_local_inds_host_view_type& gblColInds, | |
| nonconst_local_inds_host_view_type& indices, | |
| /// \param numIndices [out] Number of indices returned in \c indices. | |
| /// | |
| /// This method throws std::runtime_error if \c indices is not large | |
| /// enough to hold the column indices in row \c localRow. If row | |
| /// <tt>localRow</tt> does not belong to this process, then | |
| /// <tt>indices</tt> is not modified and \c numIndices is set to | |
| /// Teuchos::OrdinalTraits<size_t>::invalid() on output. | |
| virtual void | |
| getLocalRowCopy(local_ordinal_type localRow, | |
| nonconst_local_inds_host_view_type& indices, |
Signed-off-by: Christian Glusa <caglusa@sandia.gov>
csiefer2
force-pushed
the
pr-mirror-15184
branch
from
d456027 to
306b558
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request performs extensive documentation updates and parameter renaming across the Ifpack2 and Tpetra packages to improve clarity and consistency. Key changes include correcting file headers, fixing typos, and ensuring that Doxygen tags accurately reflect function signatures. The review feedback identifies several locations where descriptive notes and post-conditions were not updated to match the renamed parameters, such as 'NumEntries' and 'LocalRow', which is necessary to maintain documentation integrity and avoid Doxygen warnings.
| returned as Teuchos::OrdinalTraits<size_t>::invalid(). | ||
| */ | ||
| Note: A std::runtime_error exception is thrown if either \c Indices or \c Values is not large enough to hold the data associated | ||
| with row \c LocalRow. If \c LocalRow is not valid for this node, then \c Indices and \c Values are unchanged and \c NumIndices is |
There was a problem hiding this comment.
The parameter NumIndices was renamed to NumEntries on line 176. This reference in the note should be updated to maintain consistency and avoid Doxygen warnings.
| with row \c LocalRow. If \c LocalRow is not valid for this node, then \c Indices and \c Values are unchanged and \c NumIndices is | |
| with row \c LocalRow. If \c LocalRow is not valid for this node, then \c Indices and \c Values are unchanged and \c NumEntries is |
| \param indices - (Out) Local column indices corresponding to values. | ||
| \param values - (Out) Row values | ||
| \pre <tt>isGloballyIndexed() == false</tt> | ||
| \post <tt>indices.size() == getNumEntriesInDropRow(DropRow)</tt> |
There was a problem hiding this comment.
The parameter DropRow was renamed to LocalRow on line 206. The \post condition should be updated to reflect this change.
| \post <tt>indices.size() == getNumEntriesInDropRow(DropRow)</tt> | |
| \post <tt>indices.size() == getNumEntriesInDropRow(LocalRow)</tt> |
| \pre <tt>isGloballyIndexed() == false</tt> | ||
| \post <tt>indices.size() == getNumEntriesInDropRow(DropRow)</tt> | ||
|
|
||
| Note: If \c DropRow does not belong to this node, then \c indices is set to null. |
There was a problem hiding this comment.
The parameter DropRow was renamed to LocalRow on line 206. This reference in the note should be updated.
| Note: If \c DropRow does not belong to this node, then \c indices is set to null. | |
| Note: If \c LocalRow does not belong to this node, then \c indices is set to null. |
| returned as Teuchos::OrdinalTraits<size_t>::invalid(). | ||
| */ | ||
| Note: A std::runtime_error exception is thrown if either \c Indices or \c Values is not large enough to hold the data associated | ||
| with row \c GlobalRow. If \c GlobalRow does not belong to this node, then \c Indices and \c Values are unchanged and \c NumIndices is |
There was a problem hiding this comment.
The parameter NumIndices was renamed to NumEntries on line 189. This reference in the note should be updated.
| with row \c GlobalRow. If \c GlobalRow does not belong to this node, then \c Indices and \c Values are unchanged and \c NumIndices is | |
| with row \c GlobalRow. If \c GlobalRow does not belong to this node, then \c Indices and \c Values are unchanged and \c NumEntries is |
| returned as Teuchos::OrdinalTraits<size_t>::invalid(). | ||
| */ | ||
| Note: A std::runtime_error exception is thrown if either \c Indices or \c Values is not large enough to hold the data associated | ||
| with row \c LocalRow. If \c LocalRow is not valid for this node, then \c Indices and \c Values are unchanged and \c NumIndices is |
There was a problem hiding this comment.
The parameter NumIndices was renamed to NumEntries on line 206. This reference in the note should be updated.
| with row \c LocalRow. If \c LocalRow is not valid for this node, then \c Indices and \c Values are unchanged and \c NumIndices is | |
| with row \c LocalRow. If \c LocalRow is not valid for this node, then \c Indices and \c Values are unchanged and \c NumEntries is |
| returned as Teuchos::OrdinalTraits<size_t>::invalid(). | ||
| */ | ||
| Note: A std::runtime_error exception is thrown if either \c Indices or \c Values is not large enough to hold the data associated | ||
| with row \c LocalRow. If \c LocalRow is not valid for this node, then \c Indices and \c Values are unchanged and \c NumIndices is |
There was a problem hiding this comment.
The parameter NumIndices was renamed to NumEntries on line 162. This reference in the note should be updated.
| with row \c LocalRow. If \c LocalRow is not valid for this node, then \c Indices and \c Values are unchanged and \c NumIndices is | |
| with row \c LocalRow. If \c LocalRow is not valid for this node, then \c Indices and \c Values are unchanged and \c NumEntries is |
| returned as Teuchos::OrdinalTraits<size_t>::invalid(). | ||
| */ | ||
| Note: A std::runtime_error exception is thrown if either \c Indices or \c Values is not large enough to hold the data associated | ||
| with row \c GlobalRow. If \c GlobalRow does not belong to this node, then \c Indices and \c Values are unchanged and \c NumIndices is |
There was a problem hiding this comment.
The parameter NumIndices was renamed to NumEntries on line 164. This reference in the note should be updated.
| with row \c GlobalRow. If \c GlobalRow does not belong to this node, then \c Indices and \c Values are unchanged and \c NumIndices is | |
| with row \c GlobalRow. If \c GlobalRow does not belong to this node, then \c Indices and \c Values are unchanged and \c NumEntries is |
| returned as Teuchos::OrdinalTraits<size_t>::invalid(). | ||
| */ | ||
| Note: A std::runtime_error exception is thrown if either \c Indices or \c Values is not large enough to hold the data associated | ||
| with row \c DropRow. If \c DropRow is not valid for this node, then \c Indices and \c Values are unchanged and \c NumIndices is |
There was a problem hiding this comment.
The parameter DropRow was renamed to LocalRow on line 178, and NumIndices was renamed to NumEntries on line 181. This note should be updated to reflect these changes.
| with row \c DropRow. If \c DropRow is not valid for this node, then \c Indices and \c Values are unchanged and \c NumIndices is | |
| with row \c LocalRow. If \c LocalRow is not valid for this node, then \c Indices and \c Values are unchanged and \c NumEntries is |
| \param indices - (Out) Local column indices corresponding to values. | ||
| \param values - (Out) Row values | ||
| \pre <tt>isGloballyIndexed() == false</tt> | ||
| \post <tt>indices.size() == getNumEntriesInDropRow(DropRow)</tt> |
There was a problem hiding this comment.
The parameter DropRow was renamed to LocalRow on line 211. The \post condition should be updated.
| \post <tt>indices.size() == getNumEntriesInDropRow(DropRow)</tt> | |
| \post <tt>indices.size() == getNumEntriesInDropRow(LocalRow)</tt> |
| \pre <tt>isGloballyIndexed() == false</tt> | ||
| \post <tt>indices.size() == getNumEntriesInDropRow(DropRow)</tt> | ||
|
|
||
| Note: If \c DropRow does not belong to this node, then \c indices is set to null. |
There was a problem hiding this comment.
The parameter DropRow was renamed to LocalRow on line 211. This reference in the note should be updated.
| Note: If \c DropRow does not belong to this node, then \c indices is set to null. | |
| Note: If \c LocalRow does not belong to this node, then \c indices is set to null. |
Signed-off-by: Christian Glusa <caglusa@sandia.gov>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request performs an extensive cleanup of Doxygen documentation across the ifpack2 and tpetra packages, addressing numerous typos, missing parameter descriptions, and deprecated configuration tags. The changes improve documentation accuracy and resolve various Doxygen warnings. Review comments identified several remaining typos in the documentation for DropFilter, ReorderFilter, and SparsityFilter, specifically recommending the correction of isDroplyIndexed to isLocallyIndexed and getNumEntriesInDropRow to getNumEntriesInLocalRow to match the actual class interfaces.
| \post <tt>indices.size() == getNumEntriesInGlobalRow(GlobalRow)</tt> | ||
| \param indices - (Out) Global column indices corresponding to values. | ||
| \param values - (Out) Row values | ||
| \pre <tt>isDroplyIndexed() == false</tt> |
There was a problem hiding this comment.
The method isDroplyIndexed() appears to be a typo for isLocallyIndexed(). Since this PR aims to fix Doxygen warnings and improve documentation quality, correcting this typo will ensure the documentation accurately reflects the class interface.
| \pre <tt>isDroplyIndexed() == false</tt> | |
| \pre <tt>isLocallyIndexed() == false</tt> |
| \param indices - (Out) Local column indices corresponding to values. | ||
| \param values - (Out) Row values | ||
| \pre <tt>isGloballyIndexed() == false</tt> | ||
| \post <tt>indices.size() == getNumEntriesInDropRow(LocalRow)</tt> |
There was a problem hiding this comment.
The method getNumEntriesInDropRow() appears to be a typo for getNumEntriesInLocalRow(). This likely occurred during a search-and-replace operation. Correcting it will improve documentation clarity and accuracy.
| \post <tt>indices.size() == getNumEntriesInDropRow(LocalRow)</tt> | |
| \post <tt>indices.size() == getNumEntriesInLocalRow(LocalRow)</tt> |
| \param indices - (Out) Local column indices corresponding to values. | ||
| \param values - (Out) Row values | ||
| \pre <tt>isGloballyIndexed() == false</tt> | ||
| \post <tt>indices.size() == getNumEntriesInDropRow(LocalRow)</tt> |
There was a problem hiding this comment.
The method getNumEntriesInDropRow() appears to be a typo for getNumEntriesInLocalRow(). This seems to be a copy-paste error from another filter class and should be corrected to match the actual API.
| \post <tt>indices.size() == getNumEntriesInDropRow(LocalRow)</tt> | |
| \post <tt>indices.size() == getNumEntriesInLocalRow(LocalRow)</tt> |
| \post <tt>indices.size() == getNumEntriesInGlobalRow(GlobalRow)</tt> | ||
| \param indices - (Out) Global column indices corresponding to values. | ||
| \param values - (Out) Row values | ||
| \pre <tt>isDroplyIndexed() == false</tt> |
There was a problem hiding this comment.
The method isDroplyIndexed() appears to be a typo for isLocallyIndexed(). Correcting this will ensure the Doxygen documentation is accurate and consistent with the rest of the codebase.
| \pre <tt>isDroplyIndexed() == false</tt> | |
| \pre <tt>isLocallyIndexed() == false</tt> |
| \param indices - (Out) Local column indices corresponding to values. | ||
| \param values - (Out) Row values | ||
| \pre <tt>isGloballyIndexed() == false</tt> | ||
| \post <tt>indices.size() == getNumEntriesInDropRow(LocalRow)</tt> |
There was a problem hiding this comment.
The method getNumEntriesInDropRow() appears to be a typo for getNumEntriesInLocalRow(). This is likely a result of an incorrect search-and-replace operation and should be fixed to avoid confusion.
| \post <tt>indices.size() == getNumEntriesInDropRow(LocalRow)</tt> | |
| \post <tt>indices.size() == getNumEntriesInLocalRow(LocalRow)</tt> |
2 participants
Automated mirror of upstream PR trilinos#15184 @trilinos/tpetra @trilinos/ifpack2
Motivation
Fix some doxygen warnings.