Remove duplication and improve reuse of MiniTensor code in Trilinos

[bartlettroscoe]

CC: @amota, @trilinos/intrepid2

Description:

When Intrepid2 was created, it duplicated the MiniTensor code and created a maintenance problem.

This Story is to examine ways to eliminate the duplication of the MiniTensor code and improve its reuse in Trilinos and other projects.

Tasks:

1. Get copyright for MiniTensor [Done]
2. Create a new TriBITS Trilinos PT package called "MiniTensor" under packages/minitensor/ and get it working (but no other Trilinos packages use this yet) (see below) [Done]
3. Convert ROL to use new MiniTensor package [Done]
4. Convert Albany to use new MiniTensor package [Done]
5. Eliminate compiler warnings from MiniTensor package (needed for Sierra) [Done]
6. Remove version of MiniTensor in Intrepid2 package [Done]
7. Get Sierra to use new MiniTensor package [Done]
8. Remove version of MiniTensor in Intrepid [Done]

[bartlettroscoe]

Below is an email thread that I had with @amota about options for dealing with MiniTensor.

The next step will be to call a short meeting of interested Intrepid and Intrepid2 developers and stakeholders for decide how to deal with the duplication of MiniTensor (and perhaps other duplicated code?).

@amota, given that this is your code and you know these developers and stakeholders the best, could you call this meeting and add me to the attendee's list?

---

From: Mota, Alejandro
Sent: Monday, September 12, 2016 5:24 PM
To: Bartlett, Roscoe A
Subject: Re: MiniTensor

Hi Ross,

Given that MiniTensor cannot go into Teuchos because it depends on Sacado, it
seems that the most attractive solution would be to create an
IntrepidMiniTensor subpackage to avoid code duplication, as you say.

I agree then that the intrepid/intrepid2 developers should be involved in the
discussion, along with some others that care about this like Brent Perschbacher,
Dave Littlewood and Jake Ostien.

Yes, feel free to open the issue on the Trilinos GitHub site.

Thanks,

Alejandro

---

From: Bartlett, Roscoe A
Sent: Monday, September 12, 2016 5:09 PM
To: Mota, Alejandro
Subject: RE: MiniTensor

Alejandro,

Having MiniTenosr depending on Sacado would prevent it from being added to
Teuchos since Sacado depends on Teuchos and we can't have circular
dependencies. Could MiniTensor go into Scacdo? We would have to discuss
that with the owner Eric Phipps. But I think that AD and tensors are different
enough I would say that Sacado is would not be the right place to put
MiniTensor. From a conceptual standpoint, MiniTensor is a subset of the scope
for Intrepid and Intrepid2 so that seems like the right place if you are going to
put it into an existing Trilinos package.

That takes us back to the Intrepid subpackage solution. I just realized a way to
put MiniTensor under packages/intrepid/ without having to break Intrepid into
subpackages and not creating what appears to most as a new top-level Trilinos
package.

The idea is that we could just create a new top-level TriBITS package for
MiniTensor called "IntrepidMiniTensor" located under the 'intrepid' source
directory like:

  Trilinos/
    packages/
      intrepid/
        minitensor/
          src/
          test/

We would then list the new IntrepidMiniTensor library in the
Trilinos/PackagesList.cmake file as:

    ...
    IntrepidMiniTensor  ...
    Intrepid ...
    Intrepid2 ...
    ...

and then the Intrepid, Intrepid2, and ROL Dependencies.cmake files would list
"IntrepidMiniTensor" as an upstream package.

That would have all the correct behaviors w.r.t. to the checkin-test.py and CI
testing processes where a change to any file under the directory
Trilinos/packages/intrepid/minitensor/ would trigger the enable of the
IntrepidMiniTensor Package as well as the Intrepid package (and all of their
downstream dependent packages including Intrepid2, ROL, etc.). Having
MiniTensor under the intrepid/ directory would make it look like we were not
created a new top-level package.

But this is technically still created a new top-level Trilinos package and having
"IntrepidMiniTensor" not be a real subpackage under "Intrepid" will likely
confuse some people (but without harm I suspect).

To avoid those problems, we come back to the idea of officially breaking
Intrepid into the subpackages "MiniTensor" and "Core" and do as we discussed
over the phone. Or, more logically, combine Intrepid and Intrepid2 into a single
top-level package "Intrepid" with the subpackages "IntrepidMiniTensor",
"IntrepidClassic" (the old "Intrepid") and "Intrepid2" (i.e. subpackage "2" under
"Intrepid").

However, the best solution from a clarity and dependency-management
standpoint would be to make MiniTensor its own top-level package but that
would require broader approval and DOE copyright assertion. However, the
"IntrepidMiniTensor" top-level package solution I describe above requires the
most minimal changes so is attractive.

[...]

It might be good to get the Intrepid and Intrepid2 developers and stakeholders
together for a meeting for how to address this duplication.

Do mind if we create a Trilinos GitHub issue for "Remove duplication and
improve reuse of MiniTensor code in Trilinos" and then track this conversation
on their?

Cheers,

-Ross

---

From: Mota, Alejandro
Sent: Monday, September 12, 2016 4:13 PM
To: Bartlett, Roscoe A
Subject: MiniTensor

Hi Ross,

Thanks for the info you provided at the telecon.

I just remembered that MiniTensor also depends on Sacado for automatic
differentiation.

Not sure if that would prevent it from moving into Teuchos.

Alejandro

[rppawlo]

Alejandro,

Please add me to the meeting too.

Roger

[bartlettroscoe]

The decision was to try to make MiniTensor its own Trilinos/TriBITS package. See below email just sent out.

-----Original Message-----
From: Mota, Alejandro
Sent: Thursday, September 15, 2016 4:43 PM
To: [email protected]
Cc: [email protected]; Bartlett, Roscoe A; Perschbacher,
Brent M; Perego, Mauro (-EXP); Willenbring, James M; Littlewood, David John;
Ostien, Jakob T
Subject: MiniTensor: request to make it a separate package

Hello all,

More than three and a half years have passed since I submitted a request for
inclusion of MiniTensor in Trilinos, as shown below. In the end MiniTensor was
included as part of Intrepid, and it is now part of both Intrepid and Intrepid2.

In that time MiniTensor and its user base have grown. Today I had a
videoconference with Ross, Brent, Mauro, Dave Littlewood & Jake Ostien about
the future of MinTensor. For the reasons described below, the consensus was
that it would be a good idea to make MiniTensor into its own separate package.
So I’m formally requesting this.

• Proposed package name:

MiniTensor. This name is descriptive of the code’s purpose: manipulation and
operations of small vectors and tensors.

• Purpose of package:

Algebra and calculus of small vectors and tensors, most of the time of
dimensions 2 and 3, sometimes up to 20 or 30. Common transformations,
factorizations and geometric operations used in computational mechanics.
Material modeling in solid mechanics. Solution and optimization of small linear
and nonlinear systems for use in modeling of material models in solid
mechanics. Optimized for ease of use (i.e. implementation of material models
follows closely their description in papers/textbooks) and small tensors and
vectors. Provide ability to run material models in GPU/many core architectures.

• Whether to be included in an existing Trilinos package:

MiniTensor’s functionality does not exist in other packages, but it was originally
included as part of Intrepid. This was done to avoid creating a new package.
With the creation of Intrepid2, however, there are now two versions of
MiniTensor that are essentially the same. This is unnecessary code duplication.
Thus MiniTensor is now part of Intrepid/Intrepid2, but it does not depend on
them. Likewise, Intrepid/Intrepid2 do not depend on MiniTensor. They are
independent from one another.

The main codes that use MiniTensor are Albany and Sierra/Solid Mechanics. One
of the original reasons to move MiniTensor from Albany to Trilinos was to make
it available for use in both Albany and Sierra/SM. When Intrepid2 was created,
Albany started using it right away; but Sierra still uses Intrepid. Thus at the
moment it is necessary to keep two versions of MiniTensor that have the same
functionality.

Although currently MiniTensor is small (~600K), we think that it will grow in the
near future, in particular as we have started interfacing it with ROL to have
access to more sophisticated optimization algorithms for use in very complex
material models.

• Required and optional Trilinos packages:

Required: Teuchos, Sacado, Kokkos. Optional: none.

• Required and optional TPLs:

Required: boost. Optional: none.

• Stable or experimental code.

Stable as it is used in both research (Albany) and production (Sierra/SM) codes.

• Subpackages:

None.

• Development support and maintenance plan:
  • Package maintainer: Alejandro Mota, primary maintainer.
  • Release: The code is already released as part of Intrepid/Intrepid2.
  • External release, long-term maintainer: Alejandro Mota. Our solid
    mechanics research relies on using this library, thus as long as this research is
    supported in either Albany or Sierra, we will support the library.

Thanks,

Alejandro

---

On 2013/01/09, 18:46 , "Alejandro Mota" [email protected] wrote:

Hello all,

We propose a new Trilinos package as follows, according to the procedure
outlined in

https://software.sandia.gov/trilinos/developer/policies/adding_new_package.h
tml

- Proposed package name:

MikroTanustes, or in the original Greek μικρό τανυστές, which means
small tensors in English.

- Purpose of package:

Algebra and calculus of small vectors and tensors, most of the time of
dimensions 2 and 3, and definitely less than dimension 8. Common
transformations, factorizations and geometric operations used in
computational mechanics. Material modeling in solid mechanics. Optimized
for ease of use and small tensors and vectors (á la TVMet or Blitz++
small matrices).

- Whether to be included in an existing Trilinos package:

We don't see this functionality somewhere else in Trilinos. We have been
made aware of the efforts by Carter Edwards and Kokkos Arrays. There may
be some overlap there but our aim is tiny tensors and vectors that are
used for local operations.

- Required and optional Trilinos packages:

We only require Sacado for automatic differentiation. No other
requirements or options.

- Required and optional TPLs:

None other than the standard C++ libraries.

- Stable or Experimental code:

We consider it stable, and one reason we would like to see it in
Trilinos is for use in Sierra.

- Subpackages:

None

- Development support and maintenance plan:

   * Package maintainer: myself (Alejandro Mota), primary maintainer.

   * Release: code is ready for release. It is right now part of Albany
(https://software.sandia.gov/albany/) which uses Trilinos extensively.
Incidentally, the library has gone through copyright review and been
released under a BSD-like license as part of Albany.

   * External release, long-term maintainer: Alejandro Mota. Our
research relies on using this library, so as long as this research is
supported, we will support the library.

Thanks,

Alejandro

[lxmota]

Today there was a telecon about MiniTensor. In attendance Ross Bartlett, Mauro Perego, Mike Heroux, Dave Littlewood, Brent Perschbacher, and I. Here are the conclusions from that meeting:

•      Move forward with making MiniTensor its own internal Trilinos package. Avoids code duplication, frees Intrepid/Intrepid2 of extraneous code (MiniTensor and Intrepid/Intrepid2 do not depend on one another).
  

•      Start copyright process. Jim Willenbring already sent me the relevant information to start this (Thanks Jim!).
  

•      Contact Tammy Kolda about her work along these lines (done already). See if we can leverage the work of each other.
  

•      Once copyright is done (estimates vary, 2/3 months, perhaps more) create a branch in Trilinos with MiniTensor in its own directory. Push branch to main repository for review.
  

•      Open issue on Trilinos GitHub repository about this new branch and the intention to create a new MiniTensor package for others to review and comment.
  

•      Once MiniTensor becomes its own package, work with Jake, Dave and Brent to ensure that the transition goes smoothly for Sierra. Albany also uses MiniTensor but I can work on that transition.
  

So I'm closing this issue and then there will be a new one once the copyright moves forward, which will take a while.

[bartlettroscoe]

Putting back in progress since this has the right scope and background already.

[bartlettroscoe]

@lxmota,

As we discussed, please:

• Create a branch (called something like minitensor-pkg-609) and push it to this Trilinos GitHub repo so we can collaborate on it.

Then I can pull the branch and give it a try and help fix issues.

[lxmota]

@bartlettroscoe pushed initial sources to minitensor-pkg-609 branch.

[bartlettroscoe]

pushed initial sources to minitensor-pkg-609 branch.

@lxmota, thanks. I will have a look and get back to you.

[bartlettroscoe]

@lxmota, I pulled the branch minitensor-pkg-609 and it seems like there is some confusion. You are only trying to create a new TriBITS packages under the Trilinos TriBITS Repo as described here:

• https://tribits.org/doc/TribitsDevelopersGuide.html#how-to-add-a-new-tribits-package

If you look at:

• https://tribits.org/doc/TribitsDevelopersGuide.html#tribits-package-core-files

you will see that a TriBITS package does not have a PackagesList.cmake, TPLsList.cmake, or ProjectName.cmake file. It is only if you are building MiniTensor as its own TriBITS CMake project that you would define those things. But since MiniTensor has required dependencies on several Trilinos packages, that is not really a viable approach for any use case (at least not until we can do TriBITSPub/TriBITS#63).

I will go ahead and try to fix this up and if I run into problems, I will contact you. At the very least, I will push commits to the minitensor-pkg-609 branch branch that will have all of the basic TriBITS infrastructure issues resolved.

[bartlettroscoe]

@lxmota,

If fixed up MiniTensor on the minitensor-pkg-609 branch to build as its own PT package under Trilinos. I pushed the commits to the branch on this repo. I tested this locally on my machine crf450 with the checkin-test-sems.sh script with:

$ cd CHECKIN/
$  ./checkin-test-sems.sh --enable-packages=MiniTensor --local-do-all

and this passed showing:

  Configure: Passed (0.14 min)
  Build: Passed (0.39 min)
  Test: Passed (0.00 min)
  
  100% tests passed, 0 tests failed out of 2
  
  Label Time Summary:
  MiniTensor    =   0.12 sec (2 tests)
  
  Total Test time (real) =   0.07 sec

A couple of comments/suggestions:

In order to create a linkage with this GitHub issue #609, you have to put #609 with the hash # in the commit log or it will not be linked in GitHub. If you look above in this issue, you can see how my new commits with #609 are linked. You can fix your older existing commits by doing this:

$ cd Trilinos/
$ git fetch origin
$ git checkout --track origin/minitensor-pkg-609
$ git rebase -i origin/develop

Then you can "edit" the your earlier commits on the branch to change, for example:

  609: Update comments to remove references to Intrepid

to

  Update comments to remove references to Intrepid (#609)

(if you are not comfortable with that, I can do this for you.)

Another thing I noticed is that MiniTensor generates a lot of warnings (even with GCC 4.7.2) with three unique warnings:

[rabartl@crf450 MPI_RELEASE_DEBUG_SHARED_PT (master)]$ grep warning: make.out | sort | uniq
/home/rabartl/Trilinos.base/Trilinos/packages/minitensor/src/MiniTensor_LinearAlgebra.t.h:912:3: warning: unused variable ‘cosine’ [-Wunused-variable]
/home/rabartl/Trilinos.base/Trilinos/packages/minitensor/src/MiniTensor_Storage.h:266:22: warning: array subscript is above array bounds [-Warray-bounds]
/home/rabartl/Trilinos.base/Trilinos/packages/minitensor/src/MiniTensor_Storage.h:273:22: warning: array subscript is above array bounds [-Warray-bounds]

I would be good to get these cleaned up since they clutter up the build output. Also, I think that Sierra is still using -Werror to elevate warnings to errors so it would be a good idea to get these cleaned up sooner rather than later if you want Sierra to use this version of MiniTensor.

Other than that, I would say this is ready to push to the 'develop' branch. To get this onto your local 'develop' branch on your local machine, just do:

$ cd Trilinos/
$ git checkout develop
$ git merge minitensor-pkg-609
$ git pull --rebase

That will give a nice clean linear history (no need for this to be on a topic branch I don't think).

Then, if you don't mind, could you try following:

• https://github.com/trilinos/Trilinos/wiki/Local-development-with-remote-pull%2C-test%2C-and-push

and pushing this from my machine crf450 to be extra safe to avoid breaking the new CI build?

Then after that pushes, when you pull the updated 'develop' branch on your local machine, just remember to use git pull --rebase (see here) to keep a nice linear history and avoid duplicate commits.

Please let me know if you have any questions about any of this or any of the commits I made.

Thanks,

-Ross

[lxmota]

@bartlettroscoe

I'll fix the warnings. The complaint about the array bounds is a compiler bug, as I have dealt with it before and found it on gcc's bugzilla. I need to look for a workaround to eliminate the warning.

I'll go ahead and push from your machine.

[lxmota]

@bartlettroscoe I merged the minitensor-pkg-609 branch into develop and tested and pushed from crf450.

I wanted to start to fix the warnings, but when I compile in my local machine using checkin-test.py, it fails to configure because it can't find ParMETIS, which is not required by MiniTensor (or TeuchosCore, KokkosCore or Sacado, I think).

Not sure why ParMETIS is required, or how to disable it. This doesn't happen on crf450 (maybe because ParMETIS is available there?).

[bartlettroscoe]

I merged the minitensor-pkg-609 branch into develop and tested and pushed from crf450.

Confirmed. MiniTenor is showing up as its own Trilinos package in the CI build:

• http://testing.sandia.gov/cdash/index.php?project=Trilinos&parentid=2648200

I wanted to start to fix the warnings, but when I compile in my local machine using checkin-test.py, it fails to configure because it can't find ParMETIS, which is not required by MiniTensor (or TeuchosCore, KokkosCore or Sacado, I think).

ParMETIS is one of the TPLs that must be present for the new Trilinos checkin-test.py default builds. That is what #410 and #482 are about. Even if fully testing MiniTensor currently does not require ParMETIS, it will once it gets used in ROL because ROL indirectly enables Zoltan which optionally depends on ParMETIS. See:

• http://testing.sandia.gov/cdash/viewConfigure.php?buildid=2648442

which shows:

...
-- Setting Trilinos_ENABLE_MueLu=ON because ROL has an optional dependence on MueLu
...
-- Setting Trilinos_ENABLE_Zoltan=ON because MueLu has an optional dependence on Zoltan
...
-- Setting Zoltan_ENABLE_ParMETIS=ON since TPL_ENABLE_ParMETIS=ON
...

You have to explicitly enable optional TPLs or they will not get enabled. That is why ParMETIS and the other new set of Primary Tested (PT) TPLs get default enabled for the new PT CI build (see #410).

Not sure why ParMETIS is required, or how to disable it. This doesn't happen on crf450 (maybe because ParMETIS is available there?).

You can't disable ParMETIS for the --default-builds. The fact that you configure fails says that your env can't sufficiently test Trilinos for pushes. But on a machine with the SEMS env properly loaded, it is found. Therefore, it is good that the configure failed.

However, if you still want to use the chekcin-test.py script locally for your own testing and development work, you can define any --extra-builds you want and turn off the --default-builds with the checkin-test.py script. For example, you can write a checkin-test.py wrapper script like:

$ cat checkin-test-xlmota.sh
#!/bin/bash

echo "
-DTPL_ENABLE_MPI=ON
-DCMAKE_BUILD_TYPE=RELEASE
-DTrilinos_ENABLE_DEBUG=ON
-DBUILD_SHARED_LIBS=ON
-DTrilinos_ENABLE_COMPLEX=OFF
" > MPI_RELEASE_DEBUG_SHARED_ST.config

$TRILINOS_DIR/checkin-test.py \
  --default-builds= \
  --extra-builds=MPI_RELEASE_DEBUG_SHARED_ST \
  "$@"

This way, you can skip the --default-builds that require ParMETIS (or any other TPL) and define any builds you want to enable or disable anything you want. For local testing you can do:

./checkin-test-xlmota.sh --enable-packages=MiniTensor --local-do-all

But don't ever use this type of script to push to Trilinos. Only use the checkin-test-sems.sh script on a RHEL 6 machine with the SEMS env to test and push to Trilinos. Otherwise, you could break the standard PT CI build and that is bad.

Make sense? Would it help to start writing an FAQ for the checkin-test.py and checkin-test-sems.sh scripts for Trilinos to answers questions like these?

[lxmota]

Ok, thanks for the explanation.

Perhaps the easiest solution is to compile and install ParMETIS in our own local machines, so I can also test locally. Still, I promise that I'll only push from crf450.

And yes, I believe a FAQ for questions like these would be helpful, as I'm sure there will be other libraries that are needed that would seem like unexpected requirements for some new Trilinos developers.

[lxmota]

@bartlettroscoe I'm trying to convert a version of Albany to MiniTensor. There is a file MiniTensor_config.h that gets generated in the build process and placed in the Trilinos build directory under packages/minitensor/src. But that header does not get copied to the Trilinos install directory, so the Albany build fails. I compared with Intrepid2 and found a corresponding Intrepid2_config.h, but theirs gets copied to the install directory. I have compared CMakeList.txt files and I can't figure out what command or instruction to use to copy the MiniTensor_config.h header to the install directory.

[bartlettroscoe]

There is a file MiniTensor_config.h that gets generated in the build process and placed in the Trilinos build directory under packages/minitensor/src. But that header does not get copied to the Trilinos install directory, so the Albany build fails.

@lxmota,

My bad. You need to add that header file ${CMAKE_CURRENT_BINARY_DIR}/${PACKAGE_NAME}_config.h to your HEADERS variable. For an example, see:

https://github.com/trilinos/Trilinos/blob/master/packages/epetra/src/CMakeLists.txt#L39

That will result in that file getting installed.

Let me know if you have problems with this.

[lxmota]

@bartlettroscoe Thanks, trying it as we speak.

[lxmota]

Tasks 7 & 8 have been completed:
7) Port Sierra/SM to MiniTensor
8) Remove old Intrepid_MiniTensor headers from Intrepid