Skip to content

[SC-8445] fix: Apply post_process_fn to individual results in comparison tests #310

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.

Sign up for GitHub

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

Closed
juanmleng wants to merge 1 commit into from
Closed

[SC-8445] fix: Apply post_process_fn to individual results in comparison tests #310

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions validmind/tests/run.py
View file
Open in desktop
Copy link
Contributor

@johnwalz97 johnwalz97 Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure that I agree with the choice to run post-process functions for each individual result in a comparison test instead of on the final result. Although it does make sense to have the ability to do so. Maybe we should discuss in vm-library channel to see what the team thinks?

Copy link
Contributor

@AnilSorathiya AnilSorathiya Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This interesting point raised by @johnwalz97. Somewhat I agree with him. It would be clean if we keep post-process outside in the hand of users to process using the results/output of a test.

We have rawData available in the output of a test. @juanmleng can we use it in post-proces function to get desirable results?

Copy link
Contributor Author

@juanmleng juanmleng Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnwalz97 raised a very valid point here, and made me think about this a bit more. I totally agree @AnilSorathiya If we add the input_id to the RawData in our tests and leave the user to handle the combined and individual results in the post-processing, we can indeed get to the desired results.

Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ def _run_comparison_test(
params: Union[Dict[str, Any], None],
param_grid: Union[Dict[str, List[Any]], List[Dict[str, Any]], None],
title: Optional[str] = None,
post_process_fn: Optional[Callable[[TestResult], None]] = None,
):
"""Run a comparison test i.e. a test that compares multiple outputs of a test across
different input and/or param combinations"""
Expand All @@ -232,8 +233,9 @@ def _run_comparison_test(
params=params,
)

results = [
run_test(
results = []
for config in run_test_configs:
result = run_test(
test_id=test_id,
name=name,
unit_metrics=unit_metrics,
Expand All @@ -243,8 +245,9 @@ def _run_comparison_test(
generate_description=False,
title=title,
)
for config in run_test_configs
]
if post_process_fn:
result = post_process_fn(result)
results.append(result)

# composite tests have a test_id thats built from the name
if not test_id:
Expand Down Expand Up @@ -358,7 +361,9 @@ def run_test( # noqa: C901
input_grid=input_grid,
params=params,
param_grid=param_grid,
post_process_fn=post_process_fn,
)
post_process_fn = None

elif unit_metrics:
name = "".join(word.capitalize() for word in name.split())
Expand Down