Conversation
Pass conditional_names in reverse_bijective_transform
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant RB as reverse_bijective_transform
participant CT as ConditionalBijectiveTransform
participant BT as BijectiveTransform
C->>RB: Call reverse_bijective_transform(original_transform)
alt original_transform is ConditionalBijectiveTransform
RB->>CT: Instantiate ConditionalBijectiveTransform
else
RB->>BT: Instantiate BijectiveTransform
end
RB->>C: Return new transform instance
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/unit/test_transform.py (4)
21-50: Consider testing edge cases for ScaleTransform.The test logic is correct for typical positive scales. However, consider adding cases where the scale is zero or negative to ensure robust handling of unusual inputs.
80-107: Clarify naming for LogitTransform.Currently, the forward transform applies a logistic function, while the inverse applies the logit. This is logically correct code, but the “LogitTransform” name can be confusing. Consider clarifying in docstrings or renaming to avoid confusion.
108-136: Validate the log determinant in SineTransform tests.You only check that the log determinant is finite. It may be beneficial to verify that it matches the expected value (e.g., log|cos(theta)|) to confirm correctness.
137-165: Validate the log determinant in CosineTransform tests.Similar to the sine transform, consider confirming that
log_detmatches the expected derivative-based value rather than just verifying it’s finite.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/jimgw/transforms.py(1 hunks)test/unit/test_transform.py(1 hunks)
🔇 Additional comments (9)
src/jimgw/transforms.py (1)
552-558: Ensure additional attributes are preserved when reversing the transform.These lines correctly handle reversing a conditional bijective transform by checking if the original transform is ConditionalBijectiveTransform and replicating its conditional names. However, if future custom attributes are introduced for either transform, you may need to ensure they are also copied into the reversed transform.
test/unit/test_transform.py (8)
1-20: Imports and configuration look good.Enabling 64-bit mode for JAX is a standard approach for high-precision computations, and the imports are well-organized. No concerns here.
51-79: OffsetTransform tests look thorough.You are verifying both forward and inverse transformations, including JIT-compiled paths, which is comprehensive.
166-201: BoundToBound transform tests correctly handle the forward and inverse mappings.The log determinant check and JIT-compiled paths are thorough. No issues identified.
202-225: BoundToUnbound tests look correct and cover typical usage.Verifying finite log determinant is sufficient here. The coverage is good.
226-249: SingleSidedUnboundTransform tests correctly measure both forward and inverse paths.All checks, including JIT, look robust.
250-276: Alpha = -1 branch in PowerLawTransform is correctly tested.The approach of checking forward and inverse transformations is proper, and the test is well covered.
277-307: Alpha != -1.0 branch in PowerLawTransform is well validated.As with the other branch, forward and inverse tests provide confidence in correctness.
309-326: reverse_bijective_transform test is well structured.Applying the reversed transform’s forward path to the original output nicely verifies the correctness of the reversed logic. No issues found.
thomasckng
requested review from
CharmaineWONG2,
ThibeauWouters,
kazewong and
tsunhopang
|
@kazewong Ready for review. |
kazewong
left a comment
kazewong
left a comment
There was a problem hiding this comment.
Test goes thorugh. Approving this PR
kazewong
left a comment
kazewong
left a comment
There was a problem hiding this comment.
Tests go through. Approving this PR
2 participants
Summary by CodeRabbit
New Features
Tests