Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tune to pCN and rename pCN to PCN #418

Merged
merged 13 commits into from
May 21, 2024
Merged

Add tune to pCN and rename pCN to PCN #418

merged 13 commits into from
May 21, 2024

Conversation

chaozg
Copy link
Contributor

@chaozg chaozg commented May 6, 2024

fixed #415
fixed #403

What's the issue:

  • In pCNNew, tune method was empty and the unit test on warmup was omitted;
  • pCNNew should be PCNNew to keep consistency with other samplers, like ULA and LinearRTO

What I did:

  • added tune which is basically copy-paste from the old sampler;
  • initialized self.lambd in constructor;
  • activated the unit test on PCNNew's warmup in tests/zexperimental/test_mcmc.py;
  • removed PCNNew from exception for unit test on history state in tests/zexperimental/test_mcmc.py;
  • deleted a comment on checking if prior is Gaussian as it is now being checked in validate_target;
  • renamed pCNNew to PCNNew while keeping the old pCN untouched.

@chaozg chaozg changed the title Add tune to pcn Add tune to pCN and rename pCN to PCN May 6, 2024
@chaozg chaozg requested a review from nabriis May 6, 2024 12:15
Copy link
Collaborator

@nabriis nabriis left a comment

Choose a reason for hiding this comment

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

Thanks @chaozg. Glad to see the change was relatively straight forward. I had a few comments.

tests/zexperimental/test_mcmc.py Outdated Show resolved Hide resolved
tests/zexperimental/test_mcmc.py Outdated Show resolved Hide resolved
cuqi/experimental/mcmc/_pcn.py Outdated Show resolved Hide resolved
@chaozg
Copy link
Contributor Author

chaozg commented May 9, 2024

Thanks @nabriis , I've updated the codes according to your comment : )

@chaozg chaozg requested a review from nabriis May 9, 2024 20:25
Copy link
Collaborator

@nabriis nabriis left a comment

Choose a reason for hiding this comment

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

Thanks @chaozg. I could not see the state keys was updated. I also wanted to see if you could try adding this sampler to the checkpointing unit tests.

tests/zexperimental/test_mcmc.py Show resolved Hide resolved
@chaozg chaozg requested a review from nabriis May 12, 2024 20:17
Copy link
Collaborator

@nabriis nabriis left a comment

Choose a reason for hiding this comment

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

LGTM. Sorry for the slow review ;)

@jakobsj jakobsj self-requested a review May 16, 2024 06:57
Copy link
Contributor

@jakobsj jakobsj left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of small points.

cuqi/experimental/mcmc/_pcn.py Outdated Show resolved Hide resolved
cuqi/experimental/mcmc/_pcn.py Show resolved Hide resolved
@chaozg chaozg requested a review from jakobsj May 21, 2024 08:36
Copy link
Contributor

@jakobsj jakobsj left a comment

Choose a reason for hiding this comment

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

Many thanks for consulting with @furibec and adding the reference and additional explanation, as well as the new issue to revert the value. Happy for this to be merged now!

@chaozg chaozg merged commit eb95197 into main May 21, 2024
6 checks passed
@chaozg chaozg deleted the add_tune_to_pcn branch May 21, 2024 10:48
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.

PCN tune method Rename pCN to PCN
3 participants