Jmafoster1/329 estimator endpoint#382
Conversation
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports MegaLinter is graciously provided by |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #382 +/- ##
==========================================
- Coverage 97.90% 97.89% -0.01%
==========================================
Files 27 27
Lines 1572 1568 -4
==========================================
- Hits 1539 1535 -4
Misses 33 33
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
jmafoster1
force-pushed
the
jmafoster1/329-estimator-endpoint
branch
from
ea362a8 to
1458192
Compare
jmafoster1
force-pushed
the
jmafoster1/329-estimator-endpoint
branch
from
b8f0007 to
8c99927
Compare
…-project/CausalTestingFramework into jmafoster1/329-estimator-endpoint
| # Handle combined queries (global and test-specific) | ||
| test_query = test.get("query") | ||
| combined_query = None | ||
|
|
||
| if self.query and test_query: | ||
| combined_query = f"({self.query}) and ({test_query})" | ||
| logger.info( | ||
| f"Combining global query '{self.query}' with test-specific query " | ||
| f"'{test_query}' for test '{test['name']}'" | ||
| if test["estimator"] not in estimator_map: | ||
| raise ValueError( | ||
| f"Unsupported estimator {test['estimator']}. Supported: {sorted(estimator_map)}. " | ||
| "If you have implemented a custom estimator, you will need to add this to your entrypoints via your " | ||
| "pyproject.toml file." | ||
| ) | ||
| elif test_query: | ||
| combined_query = test_query | ||
| logger.info(f"Using test-specific query for '{test['name']}': {test_query}") | ||
| elif self.query: | ||
| combined_query = self.query | ||
| logger.info(f"Using global query for '{test['name']}': {self.query}") | ||
|
|
There was a problem hiding this comment.
@jmafoster1 Are we dropping functionality for test-specific queries now?
There was a problem hiding this comment.
No, the estimators all handle it anyway, so there's no need to do it here as well. The alternative would be to move the "query" argument from the estimators, which I could also do. That's possibly a bit DRY-er, but I'm not sure whether it might be nice to keep it as an option if people want to use the estimators without driving it from the main frontend?
f-allian
left a comment
f-allian
left a comment
There was a problem hiding this comment.
@jmafoster1 Generally good to go. I'll do some stress testing later but I just had a few questions for my own benefit!
2 participants
Closes #329. This PR makes estimators and expected causal effects endpoints instead of having to manually convert via a dict.