Simplify and improve wrappers #261
Conversation
The -cpp option interferes with CMake's Ninja generator.
Also simplify literal types in the test code: this cleans up a number of warnings about conversions, including an unintentional conversion to single-precision reals (the literal 3.0 was used). Also commented out unused functions that were generating warnings.
This also removes a couple of functions that were returning arrayviews *by reference* (e.g. getGlobalRowView), so the wrappers that were in place actually did nothing.
Gets rid of the 'FIXME' since SWIG now knows about Import when instantiating Export.
|
@aprokop Can you direct me how to see the test messages (and possibly the Flang backtrace) from the failing tests? They all pass on my machine... |
|
The latest build (can you not access the "Details" about to see Jenkins PR?) fails in both GCC and Flang with SegFault in |
|
Yeah I saw the "details" listing the failing tests, but I couldn't see what it reported as the failure. I don't have Docker set up on my machine, perhaps we could get together sometime and debug these failing tests on yours? |
Updated to latest SWIG. Currently fails to compile:
```
call A%getGlobalRowView(gblrow, cgview(1:nnz), csview(1:nnz))
1
Error: Type mismatch in argument 'indices' at (1); passed INTEGER(8) to CLASS(swigtype_teuchos__arrayviewt_long_long_const_t)
```
|
@sethrj oh, boy, you've been prolific! Will review this today. |
Also clearing initial pointer assignments in the test.
- Changes intent of mutable classes to intent(in) since we only modify the pointed-to-class, not the pointer - Fixes modification of "other" pointer during assignment, which would access possibly off-limits Fortran memory
|
@aprokop I think the changes in this branch might have fixed some of the crashes @mattbement (I think?) was seeing on Titan. At least, it'd be good to take another shot at running previously failing tests with this branch. |
- Updates memory management/assignment semantics - Fixes some latent type issues
There was a problem hiding this comment.
Great work! A bunch of code cleanup, and increased maintainability.
Careful review is really hard, though. For example, making sure that nothing in +-1 issues changed. Kind of wish we had all those tests in, already.
I did spend few hours going through code and commits, and am mostly OK with them.
Few issues seems to have creeped in, though, that I would like to have addressed, mostly about Kokkos:
- Removing some ignores in TpetraMap brought back the constructors with
node. We don't want any of those. - MultiVector has about 4 or 5 versions of
dot,norm*. We only want to leave one of them right now. - TpetraCrsMatrix has some constructors with Kokkos now, which is not what we want.
- I'm not sure about
Teuchos::Arrayconstructor fromTeuchos::ArrayView. Is it used somewhere? - I'd like to have a clean SWIG generation without any warnings. Right now it has some
sumInto{Local,Global}Values.
Overall, I think it's necessary to update ROADMAP.md with all the changes. It's going to take some time and effort, though, to figure out which functions were modified/added/removed.
|
@aprokop I had hoped that the maintenance gain of not having to duplicate a bunch of Tpetra constructor declarations in SWIG would outweigh the penalty of having a few useless (Kokkos node) constructors. 😔 I have previously thought about implementing a feature that would allow particular classes/types to cause declarations where they're used (such as constructors) to be ignored automatically. I think I have a means of doing this, but it's not typical/standard SWIG practice to do. That hasn't stopped us before though 😉 |
|
It may be worth noting that Trilinos seems to have (finally) adopted (re-adopted?) a release cycle. Tpetra is pushing through a bunch of deprecations now so that code can be removed this spring summer. Things on the docket for removal are:
Can anything be done now to create Tpetra wrappers without the template arguments? |
@tjfulle You mean
Not sure what you mean here. Right now we hardcode the template arguments. If Tpetra leaves just Scalar, we still need to pass that through, right? |
|
@aprokop |
|
@aprokop I think I've addressed all the feedback: everything that had Kokkos types is now gone. As to the remaining question about |
|
So the |
|
Yep! |
Infrastructure changes:
Update SWIG scripts for CMake 3.10 and higher (should still be backward compatible)
Use
.F90instead of.f90so that the compiler preprocesses automatically (newer CMake can produce a bunch of bogus warnings because it tries to apply the-cppflag to already-preprocessed code)Wrap local ordinals with native fortran
intWrap booleans with native fortran
logicalAdd typemaps for
ArrayViewto delete a lot of custom%extendcodeConvert some manual input transformations (e.g. subtracting 1) to typemaps
Remove wrapping for deprecated Tpetra functions
Clean warnings in unit tests (see Eliminate all ForTrilinos warnings #185)
Closes Change f90 to F90 #250
Closes Assignment to unallocated arrays fails in flang #211 because we're showing that the interface returns a pointer; we would rarely need to allocate a result implicitly from that returned pointer
Closes half of Implement automatic converters for ArrayView and ArrayRCP #163 : I haven't implemented the ArrayRCP yet but will soon
Closes Running
makesecond time in DeveloperMode causes recompilation of some code #127 for new versions of CMake, and I don't think we need worry about older verisons