generalized target data generation to both flu and covid

[matthewcornell]

This PR generalizes predtimechart target data generation to work with both flu and covid. The main points:

• I renamed generate_target_json_files_FluSight.py to generate_target_json_files.py.
• Its main() function takes a new ptc_config_file parameter, making the signature (hub_dir, ptc_config_file, target_out_dir). This is similar to generate_json_files.py's main() signature: (hub_dir, ptc_config_file, options_file_out, forecasts_out_dir, regenerate).
• I had to specify a schema in get_target_data_df()'s read_csv() call to force parsing the location column as String for the covid case. (Inferred schema worked for flu due to a "US" row coming sooner in the file.)
• Various function renames and consolidations into one file.
• I added an optional target_data_file_name field to predtimechart-config.yml files. See the tests for examples. Updated ptc_schema.py and HubConfig to support it.
• I did not add column name fields to it (they remain hard coded - 'date', 'value', and 'location') since they're the same for flu and covid.
• Added files and tests for covid target generation.
• Updated pyproject.toml to rename the generate_target_json_files_FluSight entry point to generate_target_json_files.
• Updated README.md.

[elray1]

Looks good to me! I had a few minor suggestions and questions.

[elray1]

There was discussion on Slack about making the target_data_file_name optional so that a hub without target data could still use the tool. My understanding is that right now, this get_target_data_df call will end up raising an error if target_data_file_name is None though? Is that right?

Handling this case seems like a low priority to me (we don't currently have users asking for this feature). Should we file an issue to handle this later, or handle it now? And if we defer handling it to later, perhaps we should just make the target_data_file_name field required for now?

[matthewcornell]

Yes, target_data_file_name is optional. If it's missing or incorrect, it should be caught by get_target_data_df(), which catches FileNotFoundError.

[elray1]

Thanks. What I'm seeing in get_target_data_df() is that it catches and then re-raises the FileNotFoundError. Will this ultimately mean that if the ptc config file doesn't contain that field, the generation of target data files will error out? I guess I was expecting to see some kind of early exit from target file creation if hub_config.target_data_file_name is None, but maybe I'm not seeing how the pieces of the full workflow fit together.

[zkamvar]

Will this ultimately mean that if the ptc config file doesn't contain that field, the generation of target data files will error out?

I was originally thinking that the FileNotFoundError is reasonable, but thinking about it from the perspective of running a pipeline, I would actually want it to emit a message saying that there is no target data to generate instead of an error because when I'm running the pipeline, I want errors to show up for hub dashboards that do specify target data. If they don't have target data to generate and an error shows up, it's just noise.

[matthewcornell]

So do you two want me to log an error and exit if the file is missing, but not actually raise an error?

[elray1]

My vote would be:

• If a file path for target data was specified but doesn't exist, we should throw an error. Something was misconfigured and we're not able to complete the task we were asked to do. We should raise an error to tell people about the problem.
• If a file path for target data was not specified (we get None here), we gracefully exit without an error. We were unable to produce target data, but we were not asked to produce target data, so there's not actually an error condition here.

[zkamvar]

I agree with Evan's breakdown.

[matthewcornell]

OK, I implemented this. I do logger.info() for the first bullet, and logger.error() for the second. In both cases I simply return, not sys.exit(). How's that?

[zkamvar]

In both cases I simply return, not sys.exit(). How's that?

Almost! If I understand correctly, both of these will run through without causing a non-zero exit code on the CLI? Instead, if you emit a logger.error(), then that should be paired with a sys.exit(1).

[matthewcornell]

Makes sense. Fixed.

[matthewcornell]

Thanks, Evan! I incorporated your suggestions.

[zkamvar]

Thank you, @matthewcornell! This looks good and is a big step toward generalization of target data generation 🎉

I have one big suggestion to not have ptc_generate_target_json_files fail if there is no target data specified in the config file.

The only other thing I would suggest (but this may be for the future): instead of having hard-coded names for date, value, and location, we could implement it as a dictionary in the config file like we do the location mappings. For example:

target_data_cols:
  date: date_onset
  value: observed
  location: province

[zkamvar]

Will this ultimately mean that if the ptc config file doesn't contain that field, the generation of target data files will error out?

I was originally thinking that the FileNotFoundError is reasonable, but thinking about it from the perspective of running a pipeline, I would actually want it to emit a message saying that there is no target data to generate instead of an error because when I'm running the pipeline, I want errors to show up for hub dashboards that do specify target data. If they don't have target data to generate and an error shows up, it's just noise.

[matthewcornell]

instead of having hard-coded names for date, value, and location, we could implement it as a dictionary in the config file like we do the location mappings

Yes, I thought of that too, but didn't document my decision. I suggest we leave it hard coded for now. I like how you used a dictionary in your example.

[zkamvar]

instead of having hard-coded names for date, value, and location, we could implement it as a dictionary in the config file like we do the location mappings

Yes, I thought of that too, but didn't document my decision. I suggest we leave it hard coded for now. I like how you used a dictionary in your example.

Would you like me to open an issue to track this feature?