search

pymc-labs / CausalPy

A Python package for causal inference in quasi-experimental settings
https://causalpy.readthedocs.io
Apache License 2.0
881 stars 63 forks source link

User specified number of significant figures for numerical outputs #283

Closed drbenvincent closed 7 months ago

drbenvincent commented 9 months ago
drbenvincent commented 9 months ago

Pfft. These doctests are brittle. Despite thinking it was a great idea, at the moment they are causing more hassle than catching actual problems. Not sure what you think @jpreszler?

jpreszler commented 9 months ago

There are definitely some that are particularly problematic, and will continue to be so without some significant changes. I think the value comes over the long-term, but it will be hard to get there without some improvements. The options I see:

  1. turn them off, at least temporarily.
  2. Split the example and output into separate docstrings (could be difficult for some longer examples). This would flag the biggest problems (an exception from input changes) but would make the actual example output static. This could be a good temporary fix in some places.
  3. Replace the more problematic examples. I generally grabbed things from the other docs which are nice realistic example but have coefficients near 0 or HDI's near or including 0. This is perhaps the biggest issue since the output may change sign in a way the makes even the pytest numeric accuracy checks too strict. It's likely possible to have examples that show the right output, but sacrifice realism for basic numeric stability that make the doctests far more reliable.

My vote would be to temporarily turn off the tests and replace the examples for at least PrePostFit (flakey), RegressionDiscontinuity (flakey) and the Instrumented Variables (slow) and turn doctests back on. I should be able to work on this soon, perhaps even getting better examples over the next week.

jpreszler commented 9 months ago

created https://github.com/pymc-labs/CausalPy/issues/284. For now, I think you can just comment out the doctest call in the CI untill that issue is resolved.

drbenvincent commented 9 months ago

Note to self: I'll finish and merge this PR after #286 is merged

codecov[bot] commented 9 months ago

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (6283c76) 76.50% compared to head (7e41931) 76.50%.

:exclamation: Current head 7e41931 differs from pull request most recent head 22f93ff. Consider uploading reports for the commit 22f93ff to get more accurate results

Files Patch % Lines
causalpy/pymc_experiments.py 41.66% 7 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #283 +/- ## ======================================= Coverage 76.50% 76.50% ======================================= Files 20 20 Lines 1345 1345 ======================================= Hits 1029 1029 Misses 316 316 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

drbenvincent commented 9 months ago

Well, I guess I have to increase test coverage to make codecov pass.