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

Allow users to access actual outcome of tests #69

Open
YYYasin19 opened this issue Nov 1, 2022 · 2 comments
Open

Allow users to access actual outcome of tests #69

YYYasin19 opened this issue Nov 1, 2022 · 2 comments

Comments

@YYYasin19
Copy link
Contributor

Hey,

this is an issue I have encountered using datajudge within some projects and I'd like to discuss whether this is a general use case.

Status Quo

Currently, the test() method of a Constraint returns a TestResult object.
Example: NumericMean

def test(self, engine: sa.engine.Engine) -> TestResult:
    # retrieve values ...
    result = deviation <= self.max_absolute_deviation
    return TestResult(result, assertion_text)

For our pytest integration, we assert the outcome of this TestResult

@pytest.mark.parametrize(
    "constraint", all_constraints, ids=Constraint.get_description
)
def test_constraint(constraint, datajudge_engine):
    # ...
    test_result = constraint.test(datajudge_engine)
    assert test_result.outcome, test_result.failure_message

This is completely fine since the goal of a test is to check whether something meets a certain requirement.
In the case of a constraint failing the user gets feedback on what went wrong in the form of the TestResult.failure_message field.

Missing feature

In general, though, datajudge might not only be used in combination with pytest to evaluate if something meets a certain requirement but also to validate the results by plotting, comparing, or evaluating them.

One concrete example is the use of statistical requirements/constraints, such as the KolmogorovSmirnov2Sample constraint where from a user perspective it might not only be important to validate whether the test result is significant but also to report back how significant it is.
A user might display these values in a dashboard tracking database changes over time, or use them to initially set some values for constraint specification.

Example: Measure how many values are currently missing in a data dump and then specify the constraint for the future based on that value.

This would, additionally, allow the user to inspect the outcomes of tests that were successful and answer the question "Okay, but how close are we to the threshold"?

Needed changes

Since the project architecture is pretty clean (🚀 ), implementing this feature would just require adding a new field to the TestResult object, e.g. values which allows the user to access the internally measured value for a certain constraint.

The change would be fully backward-compatible and could initially be just integrated for constraints where it makes sense the most, e.g. statistical constraints.
We could then leave a note in the documentation, that the results are available by accessing the field with a certain name.

For statistical constraints, this field would contain the test statistic and/or the p-value. For numeric constraints, it would contain the actually measured mean, number of rows, missing columns, etc.

Example Usage

This could result in the following code on the user side.

results = []
for year in range(2019, 2023):
  req = BetweenRequirement.from_tables(year, year+1) # simplified
  req.add_statistical_test_constraint(...)
  req.add_n_rows_max_gain_constraint(0.1)
  stat_result = req[0].test(engine)
  gain_result = req[1].test(engine)
  
   # Users can track the change in data
  results.append((year, stat_result.values["p_value"], gain_result.values["n_rows_factual"]))

It might seem like a niche use case at first, but I think it's an important part of a framework geared toward data validation. :)
Happy to hear what you think!

@kklein
Copy link
Collaborator

kklein commented Nov 2, 2022

Thank you for your write-up!

Would you mind describing why you would suggest to make the retrieved values part of the TestResult as compared to directly retrieving the results? I'm not opposed to it - I'm just curious as to what the trade-off between that and existing approach looks like.

In the example you touch upon, NumericMean, the test method doesn't do much more than calling the retrieve method with one layer of indirection. We separated the definition of retrieval from the actual testing logic for that very purpose - for many but not all constraints (yet).

Translating this to your example, the following should be possible (as of now, for most constraints):

results = []
for year in range(2019, 2023):
  req = BetweenRequirement.from_tables(year, year+1) # simplified
  req.add_statistical_test_constraint(...)
  req.add_n_rows_max_gain_constraint(0.1)
  p_value, _= req[0].retrieve(engine, req[0].ref)
  n_rows_factual, _ = req[1].retrieve(engine, req[1].ref)
  
   # Users can track the change in data
  results.append((year, p_value, n_rows_factual))

@YYYasin19
Copy link
Contributor Author

Very good point, I guess. The use case I directly had in mind used the KolmogorovSmirnov2Sample constraint which does not rely on the other internal methods, such as get_factual_values or, more importantly, retrieve which is why I didn't consider it. But you're right, that would also provide the functionality for a lot of cases. 👍

One argument that could be made in favor of storing all results would be that it is independent of how the constraint works, but the statistical tests may be the only case here, right?

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

No branches or pull requests

2 participants