-
Notifications
You must be signed in to change notification settings - Fork 21
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
Issue #604 - Add support for nominal forecasts #837
Conversation
Merge remote-tracking branch 'origin/main' into multiclass # Conflicts: # R/default-scoring-rules.R # R/validate.R
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't review closely a lot of the code related to formal S3 class setup because i'm not that familiar with the structure/functions used there. but I reviewed the tests and the general set-up with the nominal forecast type and things look good to me +/- a few very small optional suggested changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good I think and also appears correct to me. I don't have substantive comments about this PR aside from one instance of missing docs.
I did however use it to review the current changes needed to add a new class. This has improved by splitting out as_forecast but there are still a few pain points. It looks like nearly all of the me can deal with using a bit more s3 which is great.
I think we have discussed this before but I think this would be much easier to review/parse and easier for someone new to do if all the bits that defined as specific as_forecast_type where in the same file vs being split by generic method.
@seabbs some excellent points in your review here. I think moving towards I suggest addressing your points before implementing ordinal forecasts (pinging @nickreich and @elray1 for awareness) as that will make it easier to create the new ordinal class. Since Nick and Evan care about the ordinal forecasts more than the nominal ones I also suggest addressing your points before merging this. |
I don't mind either way here but I agree it would be a good idea to use the ordinal forecasts as a test case. If it were me I think I would look to merge this, make a new issue with the pain points identified, address in a PR, and then implement ordinal? |
Description
This PR closes #604.
Nominal forecasts are forecasts for outcomes that can fall in one of several unordered categories. This PR implements support for nominal forecasts (see #604, #607, and #608).
Specifically, the PR
nominal_forecast
class withassert_input_nominal
function that checks the inputs passed to a scoring functioncheck_input_nominal
, doing the same thing without producing an error - UPDATE: I think I deleted that as I didn't use it for checks. See Clean up input checks #840 for some discussion on when to check what.assert_forecast.forecast_nominal
function, checking that a data.table is complying with the required input formatmetrics_nominal
score.forecast_nominal
as_forecast()
to accept a newpredicted_label
argument.get_forecast_type()
and adds a check function to make sure that the forecast type is nominalNote:
Throughout the process, I noticed that sadly, scoringutils is currently not "easily extensible"... To make this go smoothly, there are quite a few hoops. Some of this will be simplified in the future when we implement a separate
as_forecast_nominal()
function instead of a singleas_forecast()
function that has to do all the guesswork.Still missing (likely for a future PR)
One current code example:
Checklist
lintr::lint_package()
to check for style issues introduced by my changes.