Jmafoster1/marie kondo

[jmafoster1]

Does it spark joy?

This PR gets rid of a load of relic infrastructure classes that no longer spark joy. The result is a much more streamlined testing workflow. One thing that would be nice would be to improve the structure of CausalTestCase identification and Estimator objects so that we don't need to repeat the treatment and outcome variables three times for each test.

Closes #398 #394 #392

DO NOT MERGE BEFORE #401

[github-actions]

🦙 MegaLinter status: ⚠️ WARNING

Descriptor	Linter	Files	Fixed	Errors	Elapsed time
⚠️ PYTHON	black	31		1	1.22s
✅ PYTHON	pylint	31		0	6.3s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by

[jmafoster1]

OK, so looking at the CausalTestCase class, it seems that the treatment_variable and outcome_variable properties aren't ever actually used (except in serialising to/from JSON), so we could get rid of them altogether and just store them in the associated Estimator. Programmatically this makes sense, but intuitively I'm not keen. The vibes are off - it puts too much responsibility on the "estimator". However, since the estimator also stores the adjustment set and (in the case of regression), the equational relationship, I think it might make sense to rename the Estimator family to something like CausalRelationship or something from the world of structural equational models. What are other people's thoughts?

For reference, instantiating a causal test case currently looks like this:

estimation_model = LinearRegressionEstimator(
            treatment_variable="A",
            outcome_variable="C",
            treatment_value=self.treatment_value,
            control_value=self.control_value,
            adjustment_set=self.minimal_adjustment_set,
        )
        causal_test_case = CausalTestCase(
            treatment_variable="A",
            outcome_variable="C",
            expected_causal_effect=self.expected_causal_effect,
            estimator=estimation_model,
            effect_measure="ate",
        )

In the new system, it'd look like this, which would be a lot cleaner:

causal_relationship = LinearRegressionRelationship(
            treatment_variable="A",
            outcome_variable="C",
            treatment_value=self.treatment_value,
            control_value=self.control_value,
            adjustment_set=self.minimal_adjustment_set,
        )
        causal_test_case = CausalTestCase(
            expected_causal_effect=self.expected_causal_effect,
            causal_relationship=causal_relationship,
            effect_measure="ate",
        )

[codecov]

Codecov Report

❌ Patch coverage is 94.65875% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.57%. Comparing base (3e6fd99) to head (190aa6e).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
causal_testing/discovery/abstract_discovery.py	80.00%	3 Missing ⚠️
causal_testing/estimation/abstract_estimator.py	76.92%	3 Missing ⚠️
causal_testing/specification/causal_dag.py	95.16%	3 Missing ⚠️
causal_testing/testing/causal_test_case.py	91.89%	3 Missing ⚠️
causal_testing/testing/causal_effect.py	96.29%	2 Missing ⚠️
causal_testing/causal_testing_framework.py	97.14%	1 Missing ⚠️
...esting/estimation/abstract_regression_estimator.py	97.67%	1 Missing ⚠️
...ting/estimation/instrumental_variable_estimator.py	75.00%	1 Missing ⚠️
causal_testing/testing/data_adequacy.py	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #402      +/-   ##
==========================================
- Coverage   98.18%   96.57%   -1.61%     
==========================================
  Files          32       26       -6     
  Lines        1704     1547     -157     
==========================================
- Hits         1673     1494     -179     
- Misses         31       53      +22     

Files with missing lines	Coverage Δ
causal_testing/__main__.py	99.15% <100.00%> (+3.41%)	⬆️
causal_testing/discovery/hill_climber_discovery.py	100.00% <100.00%> (+1.78%)	⬆️
...ausal_testing/estimation/cubic_spline_estimator.py	93.10% <100.00%> (-4.20%)	⬇️
...ausal_testing/estimation/experimental_estimator.py	100.00% <100.00%> (ø)
causal_testing/estimation/ipcw_estimator.py	99.25% <ø> (-0.02%)	⬇️
..._testing/estimation/linear_regression_estimator.py	100.00% <100.00%> (ø)
causal_testing/testing/causal_test_result.py	100.00% <100.00%> (ø)
causal_testing/causal_testing_framework.py	98.13% <97.14%> (-1.87%)	⬇️
...esting/estimation/abstract_regression_estimator.py	98.71% <97.67%> (+2.42%)	⬆️
...ting/estimation/instrumental_variable_estimator.py	96.29% <75.00%> (-3.71%)	⬇️
... and 6 more

... and 1 file with indirect coverage changes

---

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 355f04d...190aa6e. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jmafoster1]

Update: Turns out removing treatment_variable and outcome_variable from CausalTestCase was really easy as nothing depended on it! I've added in treatment_variable and outcome_variable property methods for backwards compatibility and easy access to the variables, but the specification of test cases is a lot cleaner now. I think I will rename the Estimator classes to CausalRelationship to create a better intuition of what they do. This will essentially allow us to create full structural causal models!