Remove polarization_samples deprecationwarning and actually deprecate it #5205
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GarethCabournDavies
added
inference
code of conduct agreed
ahnitz
requested changes
Oct 26, 2025
| marginalize_vector_params=None, | ||
| marginalize_vector_samples=1e3, | ||
| marginalize_sky_initial_samples=1e6, | ||
| marginalize_polarization=True, |
ahnitz
reviewed
Oct 26, 2025
| self.marginalize_sky_initial_samples = \ | ||
| int(float(marginalize_sky_initial_samples)) | ||
|
|
||
| if marginalize_polarization: |
Member
There was a problem hiding this comment.
I don't think you want this section at all. The point was to just use the more general marginalize_vector setup. I think this change should just be removing options, not renaming them.
Contributor
Author
There was a problem hiding this comment.
This was added in as self.marginalize_vector_params must have 'polarization' as a key later. I'm happy to simply put this in as a no-if-statement thing if that is better?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is my attempt to remove the DeprecationWarning in setup_marginalization.
Ive added a marginalise_polarization kwarg which is True by default, and does some of the needed stuff that causes a crash later.
Standard information about the request
This is a deprecation
This change affects inference
This change: has appropriate unit tests, follows style guidelines (See e.g. PEP8), has been proposed using the contribution guidelines
This change might warrant a new release, but I'm not sure
Motivation
Deprecate the thing that's had a DeprecationWarning for a while
Contents
Remove the if statement that had the warning, add a True-by-default kwarg, and add in the stuff that was needed.
I may need to rethink this as it could break when using False, but I'm not sure. We could just do the marginalisation as default without any kwarg? This was already broken if polarization_samples wasn't in the kwargs before anyway so that could be a future fix
Links to any issues or associated PRs
#5179 #4093
Testing performed
Passes the automated tests
Additional notes
🗒️