Skip to content

fix(model): correct default-metric check in HIST and IGMTF#2239

Open
genisis0x wants to merge 1 commit into
microsoft:mainfrom
genisis0x:fix/hist-igmtf-metric-default-loss
Open

fix(model): correct default-metric check in HIST and IGMTF#2239
genisis0x wants to merge 1 commit into
microsoft:mainfrom
genisis0x:fix/hist-igmtf-metric-default-loss

Conversation

@genisis0x
Copy link
Copy Markdown

Description

Closes #2163.

metric_fn in qlib/contrib/model/pytorch_hist.py and qlib/contrib/model/pytorch_igmtf.py compares self.metric against a tuple with ==:

if self.metric == ("", "loss"):
    return -self.loss_fn(pred[mask], label[mask])

self.metric is a string (default ""), so self.metric == ("", "loss") is never true. The default / "loss" metric therefore falls through to raise ValueError("unknown metric"), breaking validation/early-stopping for these two models with the default config.

Every other torch model uses a membership test — e.g. pytorch_lstm.py:147, pytorch_alstm.py:151, pytorch_gru.py:151:

if self.metric in ("", "loss"):

Change

Change == to in in both metric_fn methods so the default metric returns the negative loss, matching the rest of the model zoo.

Testing

With metric="" (default) or "loss", metric_fn now returns -loss_fn(...) instead of raising. metric="ic" and unknown metrics are unaffected.

metric_fn in pytorch_hist.py and pytorch_igmtf.py compared self.metric
(a string, default "") against the tuple ("", "loss") with ==, which is
never true, so the default/"loss" metric fell through to
raise ValueError("unknown metric"). Every other torch model
(pytorch_lstm, pytorch_alstm, pytorch_gru, ...) uses
`if self.metric in ("", "loss")`. Align HIST and IGMTF to the same
membership test so the default metric returns the negative loss as
intended. Closes microsoft#2163.
@genisis0x
Copy link
Copy Markdown
Author

@SunsetWolf when you have a moment, would appreciate a review here. Small one-line-per-file fix: HIST and IGMTF used self.metric == ("", "loss") (a string-vs-tuple compare that's never true, so the default metric hit raise ValueError), switched to in to match pytorch_lstm/alstm/gru. CLA signed, CI green.

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.

Bug: metric_fn in HIST and IGMTF always raises ValueError for default metric

1 participant