Implement reversed and strictly positive ordered transform #7759
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7759 +/- ##
=======================================
Coverage 92.83% 92.83%
=======================================
Files 107 107
Lines 18343 18354 +11
=======================================
+ Hits 17028 17039 +11
Misses 1315 1315
🚀 New features to boost your workflow:
|
pymc/distributions/transforms.py
Outdated
| return pt.cumsum(x, axis=-1) | ||
| if self.positive: | ||
| x = pt.exp(value) | ||
| else: # Everything except the first element is positive |
There was a problem hiding this comment.
Comment is a but imprecise. The deltas are positive but the values may still be negative after the cumsum
There was a problem hiding this comment.
Changed it to hopefully be more understandable
pymc/distributions/transforms.py
Outdated
| if self.positive: | ||
| x = pt.exp(value) | ||
| else: # Everything except the first element is positive | ||
| x = pt.zeros(value.shape) |
There was a problem hiding this comment.
We should use empty, also below
There was a problem hiding this comment.
Fixed. But just out of curiosity - how and why does it matter?
There was a problem hiding this comment.
It's not a fix just an optimization. Theres was another use in the other method.
It's faster because the allocated array doesn't have to be filled with zeros.
| @@ -281,6 +281,18 @@ def test_ordered(): | |||
| vals = get_values(tr.ordered, Vector(R, 3), pt.vector, floatX(np.zeros(3))) | |||
| assert_array_equal(np.diff(vals) >= 0, True) | |||
|
|
|||
| # Check that positive=True creates positive and still ordered values | |||
There was a problem hiding this comment.
Do we have a test that checks that forward/backward are inverses + jacobian that we could parametrize with the new parameters?
There was a problem hiding this comment.
Added them. Had to adjust the check_jacobian_det to take the absolute value and to have an absolute tolerance along a relative one, because ascending=False permutes values and this leads to a negative determinant and a slightly less clean logdet computation in general as it can't just multiply the main diagonal.
ricardoV94
changed the title
velochy
force-pushed
the
generalize_odered_transform
branch
2 times, most recently
from
6f8be43 to
dc594eb
Compare
pymc/distributions/transforms.py
Outdated
| x = pt.empty(value.shape) | ||
| x = pt.set_subtensor(x[..., 0], value[..., 0]) | ||
| x = pt.set_subtensor(x[..., 1:], pt.exp(value[..., 1:])) | ||
| x = pt.cumsum(x, axis=-1) # Add deltas cumulativelyto initial value |
velochy
force-pushed
the
generalize_odered_transform
branch
from
dc594eb to
87fd3a0
Compare
velochy
force-pushed
the
generalize_odered_transform
branch
from
87fd3a0 to
97cdf4e
Compare
Description
Generalize Ordered transform to take two optional parameters:
positive: (default: False) also ensure values are positive. This has better geometry than chaining order with log-transform as it avoids double-exponentiation
ascending: (default: True) allow for both ascending and descending orders. Latter is achieved simply by reversing the vector in both forward and backward transforms.
This is motivated by a draft PR into pymc-extras where I need a default transform for a positive and descending-ordered RV (NormalSingularValue)
Related Issue
NA
Checklist
Type of change