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

Features/test deterministic guide return #1

Conversation

Andrei-Treil
Copy link

Increased coverage based on specification provided in issue #3358

Proposed Changes:

  • Added 2 new parameters return_sites and use_determinisitic_guide with 2 values each
  • Added assertions to check behavior when return_deterministic is true
    • When return_sites is empty, assert all deterministic guide sites are returned
    • When return_sites contains only 1 of 2 deterministic guide sites, assert that correct site is returned
    • When there are no deterministic guide sites, assert behavior is the same as when return_deterministic is false
  • Added assertion to check that no deterministic sites are returned when return_deterministic is false and there are deterministic sites in the guide
  • Added checks to verify correct values of deterministic guide sites "w4" and "w5" when returned depending on behavior specified above

Notes:

  • All (32) test cases pass locally
    • Unsure if including with_plate and event_shape parameters is necessary, kept them in for now
  • Assumed expected values of deterministic guide sites was to match Y
  • Was unable to fully pass mypy tests when running "make test" due to non-utf encoding present in PyTorch (PyTorch issue #124897)
    • "make format", "make lint", and running pytest on test_predictive.py all succeeded

@OlaRonning
Copy link
Member

Hi @ Andrei-Treil,
I'm sorry for not getting back to you sooner; I must have missed the notification when your PR was opened. Your test looks good. Thanks for the help!

@OlaRonning OlaRonning merged commit a6e066e into aleatory-science:feature/pred_include_deter_sites May 17, 2024
@Andrei-Treil
Copy link
Author

Hi @OlaRonning,
No worries, glad I could help!

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