128 write a guide for the new prior and naming system#129
128 write a guide for the new prior and naming system#129
Conversation
…or-and-naming-system Added some content in guide
thomasckng
changed the base branch from
98-moving-naming-tracking-into-jim-class-from-prior-class
to
jim-dev
WalkthroughThe pull request introduces a comprehensive tutorial document that explains Jim’s parameter transformation system. The document details the relationships between the prior, sampler, and model parameterizations, including practical examples and visual aids. New classes for various priors and transformation processes have been added to the codebase, expanding the system’s functionality. Additionally, the documentation configuration has been updated to include JavaScript entries for MathJax support, enhancing the rendering of mathematical expressions. Changes
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
docs/tutorials/prior_system.md (8)
1-1: Revise the document title for grammatical clarity.
The current title “# Jim's parameter transform system” uses “transform” as a noun, which may be unclear. Consider changing it to “# Jim's Parameter Transformation System” for clarity and consistency.🧰 Tools
🪛 LanguageTool
[grammar] ~1-~1: There seems to be a noun/verb agreement error. Did you mean “transforms” or “transformed”?
Context: # Jim's parameter transform system !!! warning **Heavy develop...(SINGULAR_NOUN_VERB_AGREEMENT)
8-8: Add missing punctuation for clarity.
In the sentence on line 8 describing the common prior, a comma after “minimum mass” would help clarify the point. For example:
"... with some maximum and minimum mass, one may want to define ..."🧰 Tools
🪛 LanguageTool
[uncategorized] ~8-~8: Possible missing comma found.
Context: ...choice is uniform in the component mass space with some maximum and minimum mass. One...(AI_HYDRA_LEO_MISSING_COMMA)
10-10: Ensure term consistency for describing parameter sets.
The document switches between “parametrization” and “parameterization.” It is recommended to choose one variant (e.g. “parameterization”) and use it consistently throughout the text.🧰 Tools
🪛 LanguageTool
[uncategorized] ~10-~10: Do not mix variants of the same word (‘parametrization’ and ‘parameterization’) within a single text.
Context: ...l setting, there could be three sets of parametrizations we can choose for our problem: a parame...(EN_WORD_COHERENCY)
17-17: Simplify verbose phrasing in transformation explanation.
The phrase “transform the prior to the spacezusing a transform” is a bit wordy. Consider rephrasing it to something like “map the intuitive prior into thezspace” to improve clarity and brevity.🧰 Tools
🪛 LanguageTool
[style] ~17-~17: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...t is more intuitive, then transform the prior to the spacezusing a transform. By c...(EN_WORDINESS_PREMIUM_PRIOR_TO)
19-19: Streamline expression regarding bijective transforms.
Consider rephrasing “in order to make the prior-to-sample transform work in practice” to a more concise version such as “to ensure the transform works in practice.”🧰 Tools
🪛 LanguageTool
[style] ~19-~19: Consider a shorter alternative to avoid wordiness.
Context: ...le spacezto the prior space$x$ . So in order to make the prior-to-sample transform work...(IN_ORDER_TO_PREMIUM)
44-45: Standardize compound adjective spelling.
The section header “## Multi-dimensional Priors” appears to use a hyphenated form, yet style guides generally prefer “Multidimensional Priors” as a single compound word. Consider updating for consistency.🧰 Tools
🪛 LanguageTool
[misspelling] ~44-~44: This word is normally spelled as one.
Context: ...documentation (not available yet). ### Multi-dimensional Priors When working with multi-dimensio...(EN_COMPOUNDS_MULTI_DIMENSIONAL)
[misspelling] ~45-~45: This word is normally spelled as one.
Context: ...ti-dimensional Priors When working with multi-dimensional parameter space, we will usually want t...(EN_COMPOUNDS_MULTI_DIMENSIONAL)
68-68: Ensure consistency in naming parameter sets.
The bullet point beginning “In general, there could be three sets of parametrizations…” again mixes “parametrizations” with “parameterizations.” Please pick one term (preferably “parameterizations”) and apply it uniformly throughout the document.🧰 Tools
🪛 LanguageTool
[uncategorized] ~68-~68: Do not mix variants of the same word (‘parametrization’ and ‘parameterization’) within a single text.
Context: ...n general, there could be three sets of parametrizations we need to work with in our problem: 1...(EN_WORD_COHERENCY)
31-127: Specify language identifiers for fenced code blocks.
Several fenced code blocks (e.g., around lines 31, 47, 55, 91, 104, and 122) are missing language specifiers. Adding the appropriate language (such aspythonorbash) improves syntax highlighting and complies with markdownlint standards.Example for a Python code block:
-``` +```python🧰 Tools
🪛 LanguageTool
[misspelling] ~44-~44: This word is normally spelled as one.
Context: ...documentation (not available yet). ### Multi-dimensional Priors When working with multi-dimensio...(EN_COMPOUNDS_MULTI_DIMENSIONAL)
[misspelling] ~45-~45: This word is normally spelled as one.
Context: ...ti-dimensional Priors When working with multi-dimensional parameter space, we will usually want t...(EN_COMPOUNDS_MULTI_DIMENSIONAL)
[uncategorized] ~68-~68: Do not mix variants of the same word (‘parametrization’ and ‘parameterization’) within a single text.
Context: ...n general, there could be three sets of parametrizations we need to work with in our problem: 1...(EN_WORD_COHERENCY)
🪛 markdownlint-cli2 (0.17.2)
31-31: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
47-47: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
55-55: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
91-91: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
104-104: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
122-122: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
-
docs/tutorials/likelihood_transform.pngis excluded by!**/*.png -
docs/tutorials/prior_system.jpgis excluded by!**/*.jpg -
docs/tutorials/prior_system_diagram.pngis excluded by!**/*.png -
docs/tutorials/sample_transform.pngis excluded by!**/*.png
📒 Files selected for processing (2)
-
docs/tutorials/prior_system.md(1 hunks) -
mkdocs.yml(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/tutorials/prior_system.md
[grammar] ~1-~1: There seems to be a noun/verb agreement error. Did you mean “transforms” or “transformed”?
Context: # Jim's parameter transform system !!! warning **Heavy develop...
(SINGULAR_NOUN_VERB_AGREEMENT)
[uncategorized] ~8-~8: Possible missing comma found.
Context: ...choice is uniform in the component mass space with some maximum and minimum mass. One...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~10-~10: Do not mix variants of the same word (‘parametrization’ and ‘parameterization’) within a single text.
Context: ...l setting, there could be three sets of parametrizations we can choose for our problem: a parame...
(EN_WORD_COHERENCY)
[style] ~17-~17: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...t is more intuitive, then transform the prior to the space z using a transform. By c...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[style] ~19-~19: Consider a shorter alternative to avoid wordiness.
Context: ...le space z to the prior space
(IN_ORDER_TO_PREMIUM)
[typographical] ~21-~21: Two consecutive dots
Context: ... there is no limitation on the transform.. So the transform for the likelihood is ...
(DOUBLE_PUNCTUATION)
[misspelling] ~21-~21: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...ot need to be bijective, it can even by a N-to-M transform. # Prior ## Primativ...
(EN_A_VS_AN)
[misspelling] ~44-~44: This word is normally spelled as one.
Context: ...documentation (not available yet). ### Multi-dimensional Priors When working with multi-dimensio...
(EN_COMPOUNDS_MULTI_DIMENSIONAL)
[misspelling] ~45-~45: This word is normally spelled as one.
Context: ...ti-dimensional Priors When working with multi-dimensional parameter space, we will usually want t...
(EN_COMPOUNDS_MULTI_DIMENSIONAL)
[uncategorized] ~68-~68: Do not mix variants of the same word (‘parametrization’ and ‘parameterization’) within a single text.
Context: ...n general, there could be three sets of parametrizations we need to work with in our problem: 1...
(EN_WORD_COHERENCY)
🪛 markdownlint-cli2 (0.17.2)
docs/tutorials/prior_system.md
31-31: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
47-47: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
55-55: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
91-91: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
104-104: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
122-122: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 YAMLlint (1.35.1)
mkdocs.yml
[warning] 46-46: wrong indentation: expected 4 but found 2
(indentation)
| extra_javascript: | ||
| - javascripts/mathjax.js | ||
| - https://unpkg.com/mathjax@3/es5/tex-mml-chtml.js | ||
|
|
There was a problem hiding this comment.
Fix YAML indentation for extra_javascript entries.
The static analysis indicates that list items under extra_javascript should be indented 4 spaces rather than 2. This will ensure proper YAML formatting and prevent linting issues.
Proposed diff:
-extra_javascript:
- - javascripts/mathjax.js
- - https://unpkg.com/mathjax@3/es5/tex-mml-chtml.js
+extra_javascript:
+ - javascripts/mathjax.js
+ - https://unpkg.com/mathjax@3/es5/tex-mml-chtml.js📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| extra_javascript: | |
| - javascripts/mathjax.js | |
| - https://unpkg.com/mathjax@3/es5/tex-mml-chtml.js | |
| extra_javascript: | |
| - javascripts/mathjax.js | |
| - https://unpkg.com/mathjax@3/es5/tex-mml-chtml.js |
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 46-46: wrong indentation: expected 4 but found 2
(indentation)
|
|
||
| By carrying the Jacobian of the transform, we can define the prior in the space `z` by transforming the prior in the intuitive space, $p_Z(z) = p_X(x) \left| \frac{dx}{dz} \right|$, where $x = t_2(z)$. The tricky bit here is the user often wants to start from the prior $x$ and define the transform **to** the sample space `z`, but here the transform is defined **from** the sample space `z` to the prior space $x$. So in order to make the prior-to-sample transform work in practice, the transform has to be **bijective**, which implies they are invertible and have a well-defined determinant of the Jacobian. | ||
|
|
||
| On the other hand, the likelihood is often related to a model instead of coming from a distribution, meaning as long as one can transform the parameters to feed into the model, there is no limitation on the transform.. So the transform for the likelihood is strictly from the sample space `z` to the model space $y = t_1(z)$. For this reason, the transform for the likelihood does not need to be bijective, it can even by a N-to-M transform. |
There was a problem hiding this comment.
Correct punctuation and article usage.
There is an extra period after “transform” and a minor article error. Update the sentence to remove the double dot and change “a N-to-M transform” to “an N-to-M transform.”
Proposed diff:
-... there is no limitation on the transform.. So the transform for the likelihood does not need to be bijective, it can even by a N-to-M transform.
+... there is no limitation on the transform. So the transform for the likelihood does not need to be bijective, it can even be an N-to-M transform.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| On the other hand, the likelihood is often related to a model instead of coming from a distribution, meaning as long as one can transform the parameters to feed into the model, there is no limitation on the transform.. So the transform for the likelihood is strictly from the sample space `z` to the model space $y = t_1(z)$. For this reason, the transform for the likelihood does not need to be bijective, it can even by a N-to-M transform. | |
| On the other hand, the likelihood is often related to a model instead of coming from a distribution, meaning as long as one can transform the parameters to feed into the model, there is no limitation on the transform. So the transform for the likelihood is strictly from the sample space `z` to the model space $y = t_1(z)$. For this reason, the transform for the likelihood does not need to be bijective, it can even be an N-to-M transform. |
🧰 Tools
🪛 LanguageTool
[typographical] ~21-~21: Two consecutive dots
Context: ... there is no limitation on the transform.. So the transform for the likelihood is ...
(DOUBLE_PUNCTUATION)
[misspelling] ~21-~21: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...ot need to be bijective, it can even by a N-to-M transform. # Prior ## Primativ...
(EN_A_VS_AN)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
mkdocs.yml (1)
45-47:⚠️ Potential issueFix YAML indentation for
extra_javascriptentries.According to YAML linting rules, list items should be indented with 4 spaces rather than 2. This will ensure proper YAML formatting and prevent linting issues.
Apply this diff to correct the indentation:
extra_javascript: - - javascripts/mathjax.js - - https://unpkg.com/mathjax@3/es5/tex-mml-chtml.js + - javascripts/mathjax.js + - https://unpkg.com/mathjax@3/es5/tex-mml-chtml.js🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 46-46: wrong indentation: expected 4 but found 2
(indentation)
🧹 Nitpick comments (1)
src/jimgw/prior.py (1)
278-321: Add unit tests and minor docstring improvements.
TheGaussianPriorclass implementation looks correct. However, it lacks direct test coverage. Also, the docstring's phrase “which scale and translate” can be improved grammatically to “which scales and translates.”Would you like help writing a unit test to confirm that it samples from the correct mean and standard deviation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
mkdocs.yml(1 hunks)src/jimgw/constants.py(1 hunks)src/jimgw/prior.py(4 hunks)src/jimgw/single_event/likelihood.py(1 hunks)src/jimgw/single_event/utils.py(2 hunks)src/jimgw/transforms.py(5 hunks)test/unit/test_prior.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/jimgw/single_event/likelihood.py
🧰 Additional context used
🪛 YAMLlint (1.35.1)
mkdocs.yml
[warning] 46-46: wrong indentation: expected 4 but found 2
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
🔇 Additional comments (21)
mkdocs.yml (1)
45-47: Good addition of MathJax support for mathematical expressions.The addition of MathJax scripts will enable proper rendering of mathematical notation in the documentation, which is essential for a gravitational-wave inference toolkit. This aligns well with the new prior system tutorial being added in this PR.
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 46-46: wrong indentation: expected 4 but found 2
(indentation)
src/jimgw/single_event/utils.py (2)
5-5: Consistent update to use the new constant naming convention.The import has been updated to use
MTSUNinstead ofMsun, aligning with the changes in the constants module. This is a good practice for maintaining consistency across the codebase.
502-502:Details
✅ Verification successful
Proper migration to the new constant in physical calculation.
The calculation of
v0has been correctly updated to useMTSUNinstead ofMsun, maintaining the physical meaning while following the new constant naming convention.
🏁 Script executed:
#!/bin/bash # Verify that all usages of Msun have been replaced with MTSUN in the codebase # Check if there are any remaining references to the old Msun constant echo "Checking for remaining references to Msun..." rg "Msun" --no-filename | grep -v "MTSUN" || echo "No remaining references to Msun found." # Check for all references to MTSUN to ensure consistency echo "Checking for all references to MTSUN..." rg "MTSUN" --no-filenameLength of output: 574
Migration to the new constant confirmed
The update to the calculation of
v0insrc/jimgw/single_event/utils.pycorrectly usesMTSUNinstead ofMsun, preserving the intended physical meaning. The only remaining reference to a similar name appears in commented-out code (i.e.,MsunInkg), which does not affect functionality.src/jimgw/constants.py (3)
1-1: Simplified import statement.The import has been correctly updated to only include
pcfromastropy.constants, removing the unusedcimport. This is a good practice for keeping imports clean and minimal.
4-14: Well-documented physical constants with precise values.The addition of these four fundamental physical constants with clear docstrings improves code readability and maintainability. The precise values with high decimal precision ensure accurate calculations in gravitational wave physics.
The constants follow a consistent naming convention with clear units specified in the docstrings:
C_SI: Speed of light in m/sMSUN: Nominal solar mass in kgMTSUN: Geometrised nominal solar mass in secondsMRSUN: Geometrised nominal solar mass in meters
17-17: Simplified calculation of Mpc.The calculation of
Mpchas been simplified by removing the division byc.value, which is consistent with the removal of thecimport. The new calculation directly multipliespc.valueby 1e6 to convert from parsecs to megaparsecs.test/unit/test_prior.py (7)
1-3: Added JAX dependency for enhanced testing capabilities.The addition of JAX and its numpy module enables better testing of the prior distributions, particularly for checking jittability which is crucial for performance in a JAX-based toolkit.
4-13: Comprehensive import of all prior classes for testing.The import statement has been updated to include all the prior distribution classes, including the newly added
GaussianPrior. This ensures complete test coverage for all available prior distributions.
18-18: Enable 64-bit precision for numerical stability.Setting
jax_enable_x64to True ensures that JAX operations are performed with 64-bit precision, which is crucial for numerical stability in scientific computations, especially those involving probability distributions.
36-40: Added jittability checks for LogisticDistribution.Testing that the
log_probmethod can be JIT-compiled is an excellent addition. This ensures that the distribution will work efficiently with JAX's just-in-time compilation, which is crucial for performance in sampling applications.
56-60: Consistent JIT compilation tests across all distributions.All prior distributions now have consistent tests for JIT compilation of their
log_probmethods. This ensures that all distributions will work efficiently with JAX's acceleration capabilities and maintain numerical stability under compilation.Also applies to: 79-83, 103-107, 126-130, 184-188, 210-214
146-182: Improved PowerLawPrior testing with comprehensive edge cases.The tests for
PowerLawPriorhave been significantly enhanced to cover various alpha values, including the special case of alpha = -1.0. The test now directly verifies the log probability calculations against the expected analytical formulas for different ranges of alpha, which provides much stronger validation of correctness.
190-214: Complete test coverage for the new GaussianPrior class.The addition of tests for the
GaussianPriorclass ensures that the new functionality is properly validated. The tests verify that samples and log probabilities are finite, that the log probability values match those fromscipy.stats.norm, and that thelog_probmethod can be JIT-compiled.src/jimgw/prior.py (3)
14-16: Imports look good.
The newly introduced imports forSineTransformandreverse_bijective_transformalign with their usage inCosinePrior.
366-373: Validate domain assumptions.
Usingreverse_bijective_transform(SineTransform(...))effectively redefines the transform forCosinePrior. Ensure that the domain for the underlying sine transform ([-π/2, π/2]) is enforced or clearly documented, so invalid inputs don’t cause numerical issues.
433-441: PowerLawTransform usage looks correct.
Replacing conditional transforms withPowerLawTransformis coherent. Ensure that edge cases (e.g.,alphavery close to -1.0) are properly tested, given the specialized handling in the transform’s logic.src/jimgw/transforms.py (5)
90-92: Check for potential singularities.
Taking the log of the determinant here is fine, but ensure thattransform_funcis guaranteed to be invertible in all scenarios. Singular transforms can lead to infinities or NaNs.
129-131: Same determinant caution.
The inverse transform also computes the log determinant in the same manner. Validate numeric stability and handle singular transforms gracefully.
306-332: SineTransform implementation is precise.
The forward and inverse functions correctly use sine and arcsine. The domain [-π/2, π/2] is well-documented.
335-360: CosineTransform logic is consistent.
Using cosine and arccos elegantly covers the transformation in [0, π]. This matches the docstring stipulations.
503-541: Conditional logic for alpha = -1.0 appears correct.
The exponent-based transform is an appropriate special case. For alpha near -1.0 but not exact, consider whether small floating-point deviations could cause numerical instabilities.
4 participants
Starting to construct the guide for the new prior system. @xuyuon @thomasckng Feel free to add sections to this branch
Summary by CodeRabbit
UniformPrior,SinePrior,CosinePrior,PowerLawPrior,CombinePrior, andGaussianPrior.SineTransformandCosineTransform.