Propagate eps in VRMSE and VMSE#92
Open
Adonyth wants to merge 1 commit into
Open
Conversation
VRMSE.eval and VMSE.eval accept an eps parameter but silently drop it when delegating to NRMSE.eval / NMSE.eval, so the caller's eps is replaced by the 1e-7 default. Pass eps through and add a regression test on a zero-variance target, where the result is dominated by eps. Fixes PolymathicAI#75
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #75
VRMSE.evalaccepts anepsparameter but silently drops it when delegating: its body callsNRMSE.eval(x, y, meta, norm_mode="std"), so the caller'sepsis replaced by the 1e-7 default.VMSE.evalhas the identical pattern withNMSE.eval. This is easy to confirm on a zero-variance target, where the result is dominated by theepsterm in the denominator:The fix is the minimal one: pass
eps=epsthrough in bothVRMSE.evalandVMSE.eval. I audited the other metric classes inspatial.pyfor the same pattern; the remaining delegations either propagateepsalready (NRMSE→NMSE) or delegate toMSE.eval, which never useseps, so no other call site is affected.Added a regression test in
tests/test_metric.py: on a constant target,eps=10.0must change the output ofVMSE/VRMSEand must matchNMSE/NRMSEwithnorm_mode="std"and the sameeps. The test fails on currentmasterand passes with this change (pytest tests/test_metric.py: 2 passed;ruff check/ruff formatclean under the pre-commit pinned v0.6.4).Found while running an independent quantified-impact study of the benchmark's metric design; happy to adjust to maintainer preferences.