Skip to content

Raising Exception for Missing mean_tests or std_tests in MAE Metric #1661

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

Merged
merged 2 commits into from
Jul 27, 2025

Conversation

bharath03-a
Copy link
Contributor

Description

This PR addresses issue #1627. Previously, passing tests=[...] to MAE() silently ignored the test, leading to unexpected results in reports. This could confuse users, especially when writing assertions based on the number of tests executed.

Fix

  • Added a check in the MAE class constructor to raise a ValueError if tests is provided instead of mean_tests or std_tests.
  • The exception clearly instructs the user to use mean_tests or std_tests depending on what they want to test.

I had considered using a pydantic.Config class with extra = "forbid" to prevent accidental usage. It felt like a cleaner and more generalized solution for validating input.

However, I wasn't entirely sure if this would break other internal logic or dynamic field handling elsewhere in the codebase.

If this kind of config-based validation is acceptable or preferred, I'm happy to explore replacing the current manual ValueError check with a more declarative Pydantic approach. Let me know what you think!

@emeli-dral emeli-dral merged commit 2c1081d into evidentlyai:main Jul 27, 2025
24 checks passed
@emeli-dral emeli-dral requested review from Liraim and mike0sv and removed request for Liraim July 27, 2025 18:56
@mike0sv
Copy link
Collaborator

mike0sv commented Jul 28, 2025

@bharath03-a I'd say pydantic config approach is preferred if it does not break anything =) maybe we can try to add this for Metric model and see if any test or example fail?

@bharath03-a
Copy link
Contributor Author

@mike0sv yeah, I agree too adding pydantic config makes sense. Should I go ahead try that out, since this PR is already merged?

@mike0sv
Copy link
Collaborator

mike0sv commented Jul 29, 2025

@bharath03-a Yep that would be great!

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

Successfully merging this pull request may close these issues.

3 participants