-
Notifications
You must be signed in to change notification settings - Fork 94
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
Fix validation errors #1022
Comments
@kathsherratt, @sbfnk, what did we decide should happen for negative values? Set them to 0, right? |
I see #104. I understand the reasoning but if we're trying to build a general solution, then it's difficult / kind of awkward to bake exceptions into it. If we really don't want to modify these forecasts, then we could have a list of exceptions that automatically pass the validation. |
Ah yep, we didn't have a check for <0 forecast values in the first week. So when we added that to the validation checks, we made it conditional on checking submissions after the first week. I agree that is a bit awkward to put into a generalised validation framework. As a work around, could we implement this check as a "note" rather than "fail" result? So doesn't stop the check but results in a flagged note (instead of a tick or cross). (I guess that's a bit like the "notes" produced when running CRAN package checks) Alternatively (or also), we could also just replace any <0 value in the repo to 0 and check it's okay with the teams affected - like we did in the update to round values. I think that's my preference! What do you think @sbfnk? |
But we have negative values as late as mid-May, which is much later than the first week (and later than the 2021-03-22 date mentioned in #104).
I'm against this as this kind of "soft" checks are equivalent to no check at all IMO. I guess this is what happened for the negative values in
|
On an unrelated note, should we enforce or remove the character limits in |
Yes, as you guessed, I think because it was implemented as a warning comment on the PR, not a check that would fail the overall validation. So I guess some slipped through... It looks like only 2 teams had negative forecasts, |
That sounds good to me. |
Probably a good idea to enforce - nice to have a "short" version, e.g. for the web site. |
Hi @jbracher 👋, could you have a look at the metadata of KITmetricslab-bivariate_branching please?
Thanks a lot 🙏! |
This has been fixed. |
Nice job |
Discovered with:
The text was updated successfully, but these errors were encountered: