Skip to content

Develop#47

Open
XxJuliaTruongxX wants to merge 9 commits into
nasa:developfrom
XxJuliaTruongxX:develop
Open

Develop#47
XxJuliaTruongxX wants to merge 9 commits into
nasa:developfrom
XxJuliaTruongxX:develop

Conversation

@XxJuliaTruongxX

Copy link
Copy Markdown
Contributor

No description provided.

@peleser peleser 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.

Few things to clean up

Comment thread smcpy/mcmc/vector_mcmc.py
delta = np.matmul(chol, z.T).T
return inputs + delta, cov

def multivariate_normal_logpdf_batch(self, data_points, means, covariances):

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.

"data_points" -> "x"

Comment thread smcpy/mcmc/vector_mcmc.py
new_inputs, new_log_like, new_log_priors
)

# loop through multivariate_normal.logpdf is the same result as batch version

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.

remove

),
],
)
def test_vectorized_acceptance_ratio_different_covariances(array_covs, vector_mcmc):

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.

"array_covs" -> "array_old_covs"

regularized_cov = call_args[0][0][2]
expected_regularized = old_covariance + np.eye(2) * 1e-12
np.testing.assert_array_equal(regularized_cov, expected_regularized)
scipy_old_proposal_logpdf = []

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.

Make a helper function and call it twice w/ old vs. new variables



def test_local_mcmc_proposal_check_changing_scale_factor(mocker, vector_mcmc):
mock_local_mcmc_proposal = mocker.patch(

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.

Check if you can put the return value here as an arg to patch and put a return value in mocker.Mock(return_value=...)

Comment thread smcpy/smc/mutator.py

def mutate(self, particles, num_samples):
cov = particles.compute_covariance() if self._compute_cov else None
if self.mcmc_kernel._mcmc.proposal == self.mcmc_kernel.local_mcmc_proposal:

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.

Let's move this to inside VectorMCMCKernel. Otherwise looks great.

Test will need to be associated with VectorMCMCKernel as well (should be simpler)

You can use "git revert " to undo the changes to the mutator. "git log" to find the hash.

Comment thread smcpy/utils/mcmc_utils.py
def __call__(self, inputs, scale_factor):
if inputs.shape[0] < 2:
raise ValueError("inputs parameter contains less than 2 points")
orig_inputs = inputs.copy()

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.

We need to go back to storing this as a member variable and then using it to compute pairwise kernels relative to last accepted set of particles (not proposed). Details TBD

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.

Comment thread smcpy/utils/mcmc_utils.py
raise ValueError(f"Sigma must be postive, got {sigma}")

gamma = -1 / (2 * sigma**2)
return pairwise_kernels(inputs.copy(), metric="rbf", gamma=gamma)

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.

This needs to be `pairwise_kernels(inputs.copy(), self._orig_inputs, metric='rbf', gamma=gamma).

Sigma may also need to be stored.

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.

Comment thread smcpy/utils/mcmc_utils.py
D = inputs.shape[1]
NU = 2.38 / np.sqrt(D)

rbfs = self.rbf(inputs, sigma=self.median_distance(inputs))

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.

Does sigma need to be relative to original inputs as well? @peleser

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