Add slicing for TriangularMap class

[dannys4]

This PR adds the ability to slice a TriangularMap (using a contiguous slice in Julia/Python). I added bindings for those two languages (sorry @rubiop, no matlab ), but I haven't tested Python yet.

I had to change the return type of CreateTriangular for this to actually work in practice, since you can't slice arbitrary objects of type ConditionalMapBase (or use GetComponent). If this presents a problem, or we used ConditionalMapBase for a particular purpose, then I'd be happy to figure out what the best way forward is. Tagging @mparno particularly for this reason.

[github-actions]

Test Results

         1 files  ±       0           1 suites  ±0   0s ⏱️ ±0s
     198 tests  -      34       197 ✔️  -      35  0 💤 ±0  0 ❌ ±0  1 🔥 +1 
39 003 runs   - 2 272  39 002 ✔️  - 2 273  0 💤 ±0  0 ❌ ±0  1 🔥 +1 

For more details on these errors, see this check.

Results for commit 7c4c91b. ± Comparison against base commit b6d75d3.

This pull request removes 38 and adds 4 tests. Note that renamed tests count towards both.

RunTests.global ‑ SummarizedMap
RunTests.global ‑ SummarizedMap/CoeffGrad
RunTests.global ‑ SummarizedMap/Coefficients
RunTests.global ‑ SummarizedMap/Evaluation
RunTests.global ‑ SummarizedMap/Input Gradient
RunTests.global ‑ SummarizedMap/Inverse
RunTests.global ‑ SummarizedMap/LogDeterminant
RunTests.global ‑ SummarizedMap/LogDeterminantCoeffGrad
RunTests.global ‑ SummarizedMap/LogDeterminantInputGrad
RunTests.global ‑ Test serializing 3d triangular map from MonotoneComponents/Check TriangularMap Serialization
…

RunTests.global ‑ Testing AffineMap::Slice
RunTests.global ‑ Testing TriangularMap made from smaller TriangularMaps with moveCoeffs=true/Slice
RunTests.global ‑ Testing slicing evaluation of an identity conditional map
RunTests.global ‑ Testing slicing evaluation of an identity conditional map/Using Kokkos

♻️ This comment has been updated with latest results.

[github-actions]

Test Results with Externally Built Libraries

         1 files  ±       0           1 suites  ±0   0s ⏱️ ±0s
     198 tests  -      34       197 ✔️  -      35  0 💤 ±0  0 ❌ ±0  1 🔥 +1 
39 003 runs   - 2 272  39 002 ✔️  - 2 273  0 💤 ±0  0 ❌ ±0  1 🔥 +1 

For more details on these errors, see this check.

Results for commit 7c4c91b. ± Comparison against base commit b6d75d3.

This pull request removes 38 and adds 4 tests. Note that renamed tests count towards both.

RunTests.global ‑ SummarizedMap
RunTests.global ‑ SummarizedMap/CoeffGrad
RunTests.global ‑ SummarizedMap/Coefficients
RunTests.global ‑ SummarizedMap/Evaluation
RunTests.global ‑ SummarizedMap/Input Gradient
RunTests.global ‑ SummarizedMap/Inverse
RunTests.global ‑ SummarizedMap/LogDeterminant
RunTests.global ‑ SummarizedMap/LogDeterminantCoeffGrad
RunTests.global ‑ SummarizedMap/LogDeterminantInputGrad
RunTests.global ‑ Test serializing 3d triangular map from MonotoneComponents/Check TriangularMap Serialization
…

RunTests.global ‑ Testing AffineMap::Slice
RunTests.global ‑ Testing TriangularMap made from smaller TriangularMaps with moveCoeffs=true/Slice
RunTests.global ‑ Testing slicing evaluation of an identity conditional map
RunTests.global ‑ Testing slicing evaluation of an identity conditional map/Using Kokkos

♻️ This comment has been updated with latest results.

[mparno]

Hey @dannys4 ! I think returning the triangular map is fine, but I think we might need to revise how the slicing is being performed.

I think it might be best to implement the Slice function in the ConditionalMapBase class and to have it slice by dimension, not by the potentially vector-valued components that make up the TriangularMap. I think this is more natural, although harder to implement, because it slices the map in a way that is equivalent to slicing the vector-valued output of the map. (i.e., map.Evaluate(x)[a:b] == map.Slice(a,b).Evaluate(x)). If the boundaries of the slice land in the middle of one of the component outputs, we can then slice that block, which would be possible if the Slice function was added to ConditionalMapBase What do you think?

I'll leave it up to you, but I could imagine a few different options for the default ConditionalMapBase::Slice implementation. We could leave it as pure virtual, we could throw a not implemented exception, or we could define and return a SlicedMap class that simply evaluates the full map and slices the output.

Also for future reference, it's possible to downcast a shared pointer to a parent class (e.g., ConditionalMapBase) to a child class (e.g., TriangularMap) by using something like auto childPtr = std::dynamic_pointer_cast<ChildClass>(parentPtr);.

[dannys4]

It occurred to me that ConditionalMapBase should probably have a slice function, and I quite like the equivalence of map.Evaluate(x)[a:b] == map.Slice(a,b).Evaluate(x). What do you think of two functions, Slice and BlockSlice. What I implemented here becomes BlockSlice-- map.BlockSlice(2,4) is really taking the second and third "block" of the map (python notation). This means you just have to create a new vector with a few of the same shared pointers. This would probably stay as a purely TriangularMap function, at least unless you have a better idea.

Slice would be a little more work, but would be exactly what you recommend-- you just take the subsection of ConditionalMapBases corresponding to the actual concrete outputs, then recursively call Slice on the ends (assuming they're "jagged" in a sense). This would be a virtual function, so then things like AffineMap could implement them easily.

[mparno]

That sounds good to me. This is a great way to do this too, because it will make conditioning straight forward whenever we add a Condition function to ConditionalMapBase. For example, if $x=[x_1,x_2,x_3]$ and we want to sample from the conditional distribution $p(x_2 | x_1)$ using a triangular map, map, that characterizes the joint distribution $p(x_1,x_2,x_3)$, we could simple do something like map.Slice(0,2).Condition(x1) or alternatively map.Condition(x1).Slice(0,1).

[dannys4]

For completeness (and linking a mention), this partly solves #106 , but leaves Condition on the table.

[dannys4]

@mparno need a nugget of C++ wisdom. Would you expect Slice and BlockSlice to be decorated as const functions? They theoretically don't change the contents of the map, but since everything is a shared pointer, modifying a slice will sometimes modify the original map (I would say "most likely" modifies the map, but there are easy-to-qualify conditions). I don't want to mark them as const and be misleading here.

[dannys4]

I have run into a moderately large problem-- for most cases, I end up calling std::shared_ptr<ConditionalMapBase> MonotoneComponent::Slice(int a, int b). I was thoughtlessly just returning std::shared_ptr<...>(this), which I realized presents a problem when this MonotoneComponent is in a different shared pointer that gets deleted beforehand. Currently, I believe this issue happens and segfaults the program when C++ tries double deleting the same MonotoneComponent. Honestly, though, I'm not completely sure because the segfault is pretty deep in the destruction process.

While I'm not sure about the solution, I believe creating std::shared_ptr ConditionalMapBase::Slice(int,int) and ConditionalMapBase ConditionalMapBase::SliceImpl(int,int) in ConditionalMapBase would fix it (where each child class would implement SliceImpl with an eventual default implementation in ConditionalMapBase, which just returns some kind of generic SlicedMap).

[dannys4]

see: https://en.cppreference.com/w/cpp/memory/enable_shared_from_this

[mparno]

@dannys4 Just curious, where does this stand? I have some upcoming applications where this feature would be great to have. I know things will probably be slow over the the next couple of holiday weeks, but I would be happy to help push this over the finish line.

[dannys4]

To be completely frank, this is a little lower than I'd like on my priorities list. What's left, to my knowledge, is making the MonotoneComponent inherit from the std::enable_shared_from_this. Past that, I'm not exactly sure what's left-- that's just the pressing problem, but I'm hoping that's all we need to "push it out this over the finish line". I'm trying to get distributions out the door right now, but I can focus on slicing afterward.

[github-actions]

Test Results with Bindings

  2 files  ±0    2 suites  ±0   55s ⏱️ +10s
66 tests ±0  66 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
75 runs  ±0  75 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 7c4c91b. ± Comparison against base commit b6d75d3.

♻️ This comment has been updated with latest results.