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

R-CMD-check / unixOS not passing #59

Closed
2 of 4 tasks
davidsantiagoquevedo opened this issue Jun 17, 2024 · 5 comments · Fixed by #61
Closed
2 of 4 tasks

R-CMD-check / unixOS not passing #59

davidsantiagoquevedo opened this issue Jun 17, 2024 · 5 comments · Fixed by #61
Assignees

Comments

@davidsantiagoquevedo
Copy link
Member

Please place an "x" in all the boxes that apply

  • I have the most recent version of vaccineff and R
  • I have found a bug
  • I have a reproducible example
  • I want to request a new feature

I encountered errors with snapshots when running R-CMD checks. I set a fixed seed to avoid this issue, and the tests work on Windows but not on Ubuntu/macOS.

See: https://github.com/epiverse-trace/vaccineff/actions/runs/9528509173/job/26323163065?pr=57

@jpavlich
Copy link
Member

Hi @davidsantiagoquevedo I talked to @GeraldineGomez about this and I guess the best solution is to avoid using snapshots for this test. Instead, it would be better to test that the absolute difference between values is less than a threshold (e.g. 1e-2). I you agree I will implement a helper function to do this comparison.

@davidsantiagoquevedo
Copy link
Member Author

Hi @jpavlich, thanks for the comment! Yeah, that sounds better. Let me know if you need any help.

@jpavlich
Copy link
Member

@davidsantiagoquevedo I see that you have done lots of changes to tests in the branch refac-effectiveness-matching. Since I need to modify those tests I will wait until you have updated main with those changes. Let me know when you do it

Don't worry if said test still fails, since I will fix it as soon as you update main.

@jpavlich jpavlich linked a pull request Jul 17, 2024 that will close this issue
@Bisaloo
Copy link
Member

Bisaloo commented Jul 17, 2024

Note that expect_snapshot_value() has a tolerance argument. Can we use this? It would prevent having to write custom code which then needs to be maintained.

@jpavlich
Copy link
Member

@Bisaloo good to know that. I will do the change. Thanks

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 a pull request may close this issue.

4 participants