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

implements [support mapping of tasks.json task_id values to ptc task_ids text entries #25] #26

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

matthewcornell
Copy link
Collaborator

implements [support mapping of tasks.json task_id values to ptc task_ids text entries #25]

  • I don't think there's anything particularly tricky here, beyond understanding the different data structures moving around.
  • I added covid19-forecast-hub as an example hub for variety, and b/c it's one we'll be running against soon.
  • There are a few small cleanups - renames to lower case, new lines, import order, etc.
  • Note that ptc_schema.py's task_id_text property could use additional validation ("keys are str and values are dict[str:str]").

Copy link
Member

@zkamvar zkamvar left a comment

Choose a reason for hiding this comment

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

This looks good to me. I took a stab at the JSON schema, but take with a grain of salt because I am not an expert or even a competent practitioner.

"task_id_text": {
"description": "optional mapping of predtimechart task id values to text. keys are task_ids and values are an object whose keys are ptc task_id `value`s and values are ptc task_id `text`s",
"type": "object",
# todo additional validation: keys are str and values are dict[str:str]
Copy link
Member

Choose a reason for hiding this comment

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

This is the blind leading the blind so YMMV, but if all objects are key value pairs and all keys are strings, then we can check that all values are strings.

I think we can use patternProperties to check for this.

Suggested change
# todo additional validation: keys are str and values are dict[str:str]
"patternProperties": {
"^*$": {"type": "string"}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the idea. I changed my local copy and got a bunch of errors, but this stuff can be tricky. For now, let's leave my commits as-is and then I can work on it when I have time. Thanks again.

def test_hub_config_complex_forecast_hub_no_disclaimer():
hub_dir = Path('tests/hubs/example-complex-forecast-hub')
hub_config = HubConfig(hub_dir, Path('tests/configs/example-complex-no-disclaimer.yml'))
assert hub_config.disclaimer == None
assert hub_config.disclaimer is None
Copy link
Member

Choose a reason for hiding this comment

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

oops 🙈

@matthewcornell matthewcornell merged commit a094097 into main Nov 7, 2024
1 check passed
@matthewcornell matthewcornell deleted the issue_25 branch November 7, 2024 15:18
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.

2 participants