-
Notifications
You must be signed in to change notification settings - Fork 578
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
Issue4258 remove teuchos sdmatrix inheritance #4259
Changes from all commits
0598e02
32bd71b
d630017
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -285,9 +285,10 @@ int main(int argc, char *argv[]) | |
MyOM->stream(Warnings) << " ||<Y1,X1> - <X1,Y1>|| : " << err << endl; | ||
// test s.p.d. of <Y1,X1>: try to compute a cholesky | ||
lapack.POTRF('U',yTx1.numCols(),yTx1.values(),yTx1.stride(),&info); | ||
TEUCHOS_TEST_FOR_EXCEPTION(info != 0,std::runtime_error, | ||
"New <Y1,X1> not s.p.d.: couldn't computed Cholesky: info == " << info | ||
<< "\nyTx1: \n" << yTx1); | ||
std::ostringstream msg("New <Y1,X1> not s.p.d.: couldn't computed Cholesky: info == "); | ||
msg << info << "\nyTx1: \n"; | ||
yTx1.print(msg); | ||
TEUCHOS_TEST_FOR_EXCEPTION(info != 0,std::runtime_error, msg.str()); | ||
|
||
// X2 ortho to Y1 | ||
MVT::MvRandom(*X2); | ||
|
@@ -345,9 +346,10 @@ int main(int argc, char *argv[]) | |
MyOM->stream(Warnings) << " ||<Y2,X2> - <X2,Y2>|| : " << err << endl; | ||
// test s.p.d. of <Y2,X2>: try to compute a cholesky | ||
lapack.POTRF('U',yTx2.numCols(),yTx2.values(),yTx2.stride(),&info); | ||
TEUCHOS_TEST_FOR_EXCEPTION(info != 0,std::runtime_error, | ||
"New <Y2,X2> not s.p.d.: couldn't computed Cholesky: info == " << info | ||
<< "\nyTx2: \n" << yTx2); | ||
msg.str("New <Y2,X2> not s.p.d.: couldn't computed Cholesky: info == "); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe the suggestion to implement the operator<< is exactly what Mike did in Dakota. If I added one for Teuchos::SerialDenseMatrix&, that would remove the need to touch the other Trilinos packages but in a way that has proven overly prescriptive for Dakota. Is there a more attractive approach, eg a virtual operator<< ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, is there a way to add Mike (Eldred) as a watcher of this PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rhoope You could just do an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rhoope wrote:
Is the issue that you would like Dakota to be able to override any existing print behavior of It's hard to do that when using an operator. Argument-dependent lookup means that you can't use the same input types ( // Implementation detail; not for users
template<class OrdinalType, class ScalarType>
struct SerialDenseMatrixOutputWrapper {
const SerialDenseMatrix<OrdinalType, ScalarType>& A;
};
// this is for Dakota developers
template<class OrdinalType, class ScalarType>
SerialDenseMatrixOutputWrapper<OrdinalType, ScalarType>
wrapSDM (const SerialDenseMatrix<OrdinalType, ScalarType>& A)
{
return {A};
}
// Dakota would define this
template<class OrdinalType, class ScalarType>
std::ostream& operator<< (std::ostream& out, const SerialDenseMatrixOutputWrapper<OrdinalType, ScalarType>& A)
{
// ... print A however Dakota likes to print it ...
return out;
} You would use the class this way: SerialDenseMatrix<int, double> A ( /* whatever goes here */ );
// ... do stuff with A ...
std::cout << wrapSDM (A) << std::endl; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mseldre wrote:
I very much appreciate your contributions here -- thanks!
Argument-dependent lookup (ADL) would normally pick up
I'm OK with this personally, but it does break backwards compatibility for downstream applications. It's very likely that you won't be able to test all possible combinations of build options even within Trilinos, so this may break some Trilinos packages. It may also break downstream apps. Trilinos hasn't had an official minor release (!) since 12.12, around three years ago (!!!). This means that all bets are off in terms of deprecation and removal policy. (Feel free to express your opinions about that to @maherou. I have no control over it.) Lack of policy mainly just means that you'll have to manage the angry-grams from various apps if their builds break. If you're OK with that, we can merge this and file a separate issue where you will be listed as the point of contact for explaining how to work around any build issues that arise. It's no big deal, but I just want to make sure we document why we did this, that it does break backwards compatibility, and how to fix it. Is that OK? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the pointer about ADL -- that explains what I was observing and, IMO, also strengthens the argument for not providing these operators. I can't speak to internal Trilinos policy for deprecation or release management, but seems there needs to be some mechanism to move forward. From the Dakota side, we would of course like to see our local changes to Teuchos propagate upstream so that users can build with either our version or their own custom version of Trilinos, with minimal/no inconsistencies. I am fine with delaying this patch until the appropriate time when deprecations can be announced and managed -- in the meantime, we can get by with our local version. In terms of being the POC for Trilinos angry-grams, yes if that's necessary in the short term. On a semi-related note, I was also eyeing the CompObject data that appears to go unused by SerialDense*. I added some (redundant) protection around the computation of FLOP increments since the flop accumulator that accepts them seems to always be NULL based on ctor initializers. It would be easy to manage ctor paths with and without meaningful FLOP counters, but it seems to currently be an afterthought. Am I missing context? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mseldre Thanks for the reply! In brief, I would say, go for it! :-D (Ditto for the CompObject changes, though please in a separate PR :-) .) If you're OK with getting a few questions about how to fix build errors, I'll be happy to approve this PR. Long answer: @ikalash, @rrdrake, @theguruat12, @micahahoward, @rppawlo, or @trilinos/framework may wish to chime in, but I would summarize the de facto "Trilinos versioning" policy for many internal applications as follows:
This implies that apps define their own "Trilinos versions." Apps actually use this model. That, along with open access to Trilinos' repo on GitHub, likely explains the lack of enthusiasm for official Trilinos releases. I've gotten about as many complains about deprecation warnings ("hey, we can't build warning-free") as I've gotten about actual interface changes, so it's likely less effort just to break the build than it is to do an official deprecation (how long? who knows, since no official releases!). What's missing from this model is a way for an app to work correctly with multiple Trilinos versions at once. A few packages have added this feature themselves. For example, Kokkos has a CMake option and macro to help manage deprecation. Tpetra just recently added one. It may make sense for Teuchos to add this as well, but Teuchos is a big utility package without a single focus, so it may be hard to enforce the deprecation model and decide when to remove deprecated stuff. Anyway, I hope the long explanation helps! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the explanation. Approving the PR sounds good to me. Happy to help with suggestions/examples of supplying ostream insertion operators for Vector,Matrix,SymMatrix. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just pushed some mods to make GCC 7.3.0 happy. |
||
msg << info << "\nyTx2: \n"; | ||
yTx2.print(msg); | ||
TEUCHOS_TEST_FOR_EXCEPTION(info != 0,std::runtime_error, msg.str()); | ||
} | ||
MyOM->stream(Warnings) << endl; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I'd rather not build the
std::ostringstream
each time if we only use it wheninfo
is nonzero. How about this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize now that the above code is in a test, so I'm less worried about it.