-
Notifications
You must be signed in to change notification settings - Fork 5
Jmafoster1/remove data collector #308
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
Conversation
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #308 +/- ##
==========================================
- Coverage 96.87% 94.64% -2.23%
==========================================
Files 29 25 -4
Lines 1856 1345 -511
==========================================
- Hits 1798 1273 -525
- Misses 58 72 +14
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
This PR looks to drop the overall percentage code coverage, but that's because I've ripped out a bunch of code, so the proportions have changed slightly. |
…rk into jmafoster1/remove-data-collector
Resolved merging conflicts from main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmafoster1 Thanks Michael, this all looks good. However, we should also update our documentation to reflect these changes too. If you don't have the time let me know and I'll take it on 👍🏼
Good point. Done |
I think this is ready for review now. Absolute monster of a PR! |
@jmafoster1 This looks fine now. There are still some mentions of the data collector / JSON class in our |
…project/CausalTestingFramework into jmafoster1/remove-data-collector
Thanks Farhad. I had a quick look through and removed all mentions of data collection and JSON from docs and examples by running grep for "collect" and "json" and removing all the problematic content. I agree that we can get this merged in now for the JOSS paper and give the docs a proper overhaul in the coming months, but thought it was important to lose anything that could be confusing or misleading! |
No need to re-review. Once this batch of tests pass, I'll get this PR merged in and do the JOSS release |
DataCollector
classes have been removedObservationalDataCollector
has been replaced by directly passing dataframesExperimentalDataCollector
has been replaced byExperimentalEstimator
class to directly run modelsMetamorphicRelation
since this can be done via the JSON front endMetamorphicRelation
classes into thetesting
package instead ofspecification
IPCWEstimator
) to take aBaseTestCase
instead of a treatment and outcome variable