Skip to content
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

[WIP] Complete task-schema.json #11

Merged
merged 26 commits into from
Nov 22, 2022
Merged

[WIP] Complete task-schema.json #11

merged 26 commits into from
Nov 22, 2022

Conversation

annakrystalli
Copy link
Member

@annakrystalli annakrystalli commented Nov 17, 2022

Pull request resolves:

Have also added additional field described in hubDocs and the googledoc. It did raise a question (hubverse-org/hubDocs#13) regarding the target task id which was not present in the docs but is in all the examples and two variables (outcome_variable and outcome_modifier) which were in the docs but not in any of the examples. Would be great to get some clarity on that question :)

Not sure I've captured the submission due description information correctly!

And any feedback on the waffly and perhaps imprecise description fields very welcome!

tasks-schema.json Outdated Show resolved Hide resolved
Comment on lines 162 to 194
"target_date": {
"description": "An object containing arrays of required and optional unique target dates. For short-term forecasts, the target_date specifies the date of occurrence of the outcome of interest. For instance, if models are requested to forecast the number of hospitalizations that will occur on 2022-07-15, the target_date is 2022-07-15",
"example": "2022-11-05",
"type": "object",
"properties": {
"required": {
"description": "Array of target date unique identifiers that must be present for submission to be valid. Can be null if no target dates are required and all valid target dates are specified in the optional property.",
"type": [
"array",
"null"
],
"items": {
"type": "string",
"format": "date"
}
},
"optional": {
"description": "Array of valid but not required unique target date identifiers. Can be null if all target dates are required and are specified in the required property.",
"type": [
"array",
"null"
],
"items": {
"type": "string",
"format": "date"
}
}
},
"required": [
"required",
"optional"
]
},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this "target_date" information required? My understanding is that it would be redundant with "origin_date" and "horizon".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding from the documentation on the Task ID variables in the Data Formats Overview page where target_date is listed as a task_id is that there are a few ways to specify target date and the option to include it directly is acceptable? The docs also include the following note:

We encourage hubs to avoid redundancy in the model task columns. For example, Hubs should not include all three of target_date, origin_date, and horizon as task id columns because if any two are specified, the third can be calculated directly. Similarly, if a variable is constant, it should not be included. For example, if a Hub doesn’t include multiple outcome variables, an outcome variable should not be included among the task id columns.

Perhaps a decision was made that isn't reflected in the docs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think target_date should not be required but should be one of several optional, valid ways to specify dates.

tasks-schema.json Outdated Show resolved Hide resolved
tasks-schema.json Outdated Show resolved Hide resolved
tasks-schema.json Outdated Show resolved Hide resolved
tasks-schema.json Outdated Show resolved Hide resolved
tasks-schema.json Outdated Show resolved Hide resolved
"enum": [
"numeric", "integer", "double"
]
},
"minimum": {
"description": "The minimum inclusive valid cumulative distribution function value",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "The minimum inclusive valid cumulative distribution function value",
"description": "The minimum inclusive valid cumulative distribution function value.",

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there ever a setting in which a hub would want to specify a minimum cdf value other than 0, or a maximum cdf value other than 1? My inclination is that this is kind of a strange thing to let hubs specify, but maybe I'm not thinking of the use case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. I wasn't sure whether such a use case would exist so left it as a possibility. But happy to hard code 0 & 1 into the checks

tasks-schema.json Outdated Show resolved Hide resolved
tasks-schema.json Outdated Show resolved Hide resolved
tasks-schema.json Outdated Show resolved Hide resolved
tasks-schema.json Outdated Show resolved Hide resolved
tasks-schema.json Outdated Show resolved Hide resolved
tasks-schema.json Outdated Show resolved Hide resolved
tasks-schema.json Outdated Show resolved Hide resolved
tasks-schema.json Outdated Show resolved Hide resolved
tasks-schema.json Outdated Show resolved Hide resolved
"enum": [
"numeric", "integer", "double"
]
},
"minimum": {
"description": "The minimum inclusive valid cumulative distribution function value",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there ever a setting in which a hub would want to specify a minimum cdf value other than 0, or a maximum cdf value other than 1? My inclination is that this is kind of a strange thing to let hubs specify, but maybe I'm not thinking of the use case.

tasks-schema.json Outdated Show resolved Hide resolved
tasks-schema.json Outdated Show resolved Hide resolved
Comment on lines 630 to 631
"minimum": 0,
"maximum": 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above

Suggested change
"minimum": 0,
"maximum": 1

tasks-schema.json Outdated Show resolved Hide resolved
tasks-schema.json Outdated Show resolved Hide resolved
annakrystalli and others added 10 commits November 21, 2022 13:07
Co-authored-by: Nicholas G Reich <nick@umass.edu>
Co-authored-by: Nicholas G Reich <nick@umass.edu>
Co-authored-by: Nicholas G Reich <nick@umass.edu>
Co-authored-by: Evan Ray <elray1@users.noreply.github.com>
Co-authored-by: Nicholas G Reich <nick@umass.edu>
Co-authored-by: Nicholas G Reich <nick@umass.edu>
Co-authored-by: Nicholas G Reich <nick@umass.edu>
Co-authored-by: Nicholas G Reich <nick@umass.edu>
Co-authored-by: Nicholas G Reich <nick@umass.edu>
Co-authored-by: Nicholas G Reich <nick@umass.edu>
annakrystalli and others added 6 commits November 21, 2022 13:14
Co-authored-by: Evan Ray <elray1@users.noreply.github.com>
Co-authored-by: Evan Ray <elray1@users.noreply.github.com>
Co-authored-by: Evan Ray <elray1@users.noreply.github.com>
Co-authored-by: Nicholas G Reich <nick@umass.edu>
Co-authored-by: Evan Ray <elray1@users.noreply.github.com>
Co-authored-by: Evan Ray <elray1@users.noreply.github.com>
@annakrystalli
Copy link
Member Author

Thank you for the reviews and commit suggestions @LucieContamin, @nickreich & @elray1 ! And apologies for the messy commit history from accepting them one by one 🤦‍♀️. I'll go ahead and implement some of the agreed changes.

@annakrystalli
Copy link
Member Author

As suggested by @nickreich , merging so we can start afresh using issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants