aesara-devs / aehmc

An HMC/NUTS implementation in Aesara
MIT License
33 stars 6 forks source link

Improve readability by packing frequently used variables into namedtuples. #99

Closed zoj613 closed 1 year ago

zoj613 commented 1 year ago

closes #67

Here are a few important guidelines and requirements to check before your PR can be merged:

Don't worry, your PR doesn't need to be in perfect order to submit it. As development progresses and/or reviewers request changes, you can always rewrite the history of your feature/PR branches.

If your PR is an ongoing effort and you would like to involve us in the process, simply make it a draft PR.

zoj613 commented 1 year ago

@rlouf could you please help diagnose the one failed test test_nuts_mcse? It appears that the MCSE fails for one of the cases in the NUTS sampler. The changes seem fine and do not appear to change behavior so i'm a little confused.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (75fd205) 100.00% compared to head (53f8f8b) 100.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #99 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 13 13 Lines 544 565 +21 Branches 31 31 ========================================= + Hits 544 565 +21 ``` | [Impacted Files](https://app.codecov.io/gh/aesara-devs/aehmc/pull/99?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aesara-devs) | Coverage Δ | | |---|---|---| | [aehmc/algorithms.py](https://app.codecov.io/gh/aesara-devs/aehmc/pull/99?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aesara-devs#diff-YWVobWMvYWxnb3JpdGhtcy5weQ==) | `100.00% <100.00%> (ø)` | | | [aehmc/hmc.py](https://app.codecov.io/gh/aesara-devs/aehmc/pull/99?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aesara-devs#diff-YWVobWMvaG1jLnB5) | `100.00% <100.00%> (ø)` | | | [aehmc/integrators.py](https://app.codecov.io/gh/aesara-devs/aehmc/pull/99?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aesara-devs#diff-YWVobWMvaW50ZWdyYXRvcnMucHk=) | `100.00% <100.00%> (ø)` | | | [aehmc/nuts.py](https://app.codecov.io/gh/aesara-devs/aehmc/pull/99?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aesara-devs#diff-YWVobWMvbnV0cy5weQ==) | `100.00% <100.00%> (ø)` | | | [aehmc/proposals.py](https://app.codecov.io/gh/aesara-devs/aehmc/pull/99?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aesara-devs#diff-YWVobWMvcHJvcG9zYWxzLnB5) | `100.00% <100.00%> (ø)` | | | [aehmc/step\_size.py](https://app.codecov.io/gh/aesara-devs/aehmc/pull/99?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aesara-devs#diff-YWVobWMvc3RlcF9zaXplLnB5) | `100.00% <100.00%> (ø)` | | | [aehmc/termination.py](https://app.codecov.io/gh/aesara-devs/aehmc/pull/99?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aesara-devs#diff-YWVobWMvdGVybWluYXRpb24ucHk=) | `100.00% <100.00%> (ø)` | | | [aehmc/trajectory.py](https://app.codecov.io/gh/aesara-devs/aehmc/pull/99?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aesara-devs#diff-YWVobWMvdHJhamVjdG9yeS5weQ==) | `100.00% <100.00%> (ø)` | | | [aehmc/window\_adaptation.py](https://app.codecov.io/gh/aesara-devs/aehmc/pull/99?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aesara-devs#diff-YWVobWMvd2luZG93X2FkYXB0YXRpb24ucHk=) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

brandonwillard commented 1 year ago

These updates look great; I'm ready to move forward with them.

zoj613 commented 1 year ago

These updates look great; I'm ready to move forward with them.

I will still need to look into updating the docstrings. @rlouf any thoughts on these changes?

brandonwillard commented 1 year ago

Merged, because these changes are already very helpful. We can add follow-ups for the docstrings and anything else.