Skip to content

Zero source rj#14

Open
davidrandell84 wants to merge 11 commits into
mainfrom
zero_source_rj
Open

Zero source rj#14
davidrandell84 wants to merge 11 commits into
mainfrom
zero_source_rj

Conversation

@davidrandell84

@davidrandell84 davidrandell84 commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

Description

Extend reversible jump code to handle reversible jump cases with no components.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Jupyter Notebooks

No changes to existing notebooks

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
Please also list any relevant details for your test configuration

  • fix_samplers has been extend to handle extra cases including any empty source case
  • test_prior_recovery has been generalised to compare to a truncated Poisson rather than a full Poisson. This wasn't a problem previously due to the density of the Poisson outside of the range being negligible.
  • test_parameter has been extended to include cases with size 0 coefficients

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@davidrandell84 davidrandell84 requested a review from mattj89 June 16, 2026 12:57
@davidrandell84 davidrandell84 marked this pull request as ready for review June 17, 2026 07:11

@mattj89 mattj89 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some docstring updates. I don't understand a lot of the code changes (e.g. returning the allocation matrix as a parameter vector from the MixtureParameter cases). Let's discuss next week.

Comment thread src/openmcmc/sampler/reversible_jump.py Outdated
parameters that need to be created/removed as part of the dimension change. The default behaviour is to
sample the necessary additional values from the associated parameter prior distribution. Defaults to None.
n_max (int): upper limit on self.param (lower limit is assumed to be 1).
n_max (int): upper limit on self.param.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add: Defaults to None..

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this for completeness but the fact is pretty much every input to RJ has None as default but is in actual fact required to make it run.

Comment thread src/openmcmc/sampler/reversible_jump.py Outdated
Comment thread src/openmcmc/sampler/reversible_jump.py Outdated
Comment thread src/openmcmc/sampler/reversible_jump.py Outdated
Comment thread src/openmcmc/sampler/reversible_jump.py Outdated
Comment thread src/openmcmc/parameter.py Outdated
"""
return sparse.diags(diagonals=state[self.param][state[self.allocation]].flatten(), offsets=0, format="csc")
if state[self.param].shape[0] == 0:
diagonals = state[self.allocation].flatten()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that self.allocation is an allocation index (i.e. index vector which determines the allocation of each element of the data to e.g. a kernel). I don't think this should just go on the diagonal of a precision matrix? Totally different to the functionality which is in the other case statement?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have updated to zeros and looked at cases where this is used and it is not used anywhere but the unit tests

Comment thread src/openmcmc/parameter.py Outdated

"""
if state[self.param].shape[0] == 0:
return state[self.allocation]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As with the matrix case (lines 503-508), why would just just set the predictor to be the allocation vector?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have updated to zeros and looked at cases where this is used and it is not used anywhere but the unit tests

Comment thread tests/test_parameter.py

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not looked at tests yet- will do so once we've had chance to discuss the above issues.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not looked at tests yet- will do so once we've had chance to discuss the above issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants