How strict should we be about composition of model_id as <team_abbr>-<model_abbr>?

[elray1]

It has surfaced that we are not really clear about whether the composition of model_id as <team_abbr>-<model_abbr> is a convention or a requirement. This touches on hubValidations, hubDocs, and hubverse-transformations.

Making this a requirement would ensure that it was always possible to split the model_id into the team and model identifiers, e.g. as done in hubUtils::model_id_split.

Making this a convention would make our setup less restrictive, enabling hubs to use other setups for their model ids or avoid relying on or using the concept of teams.

Actions to take depending on results of this decision:

• If we want this to be a requirement, we should add something validating this to hubValidations.
• Whatever decision we make, we should add some clarifications about this to hubDocs.
• This may impact some stuff that's done in hubverse-transformations -- if we say this is just a convention, that functionality would not be able to perform the split operation reliably.

[annakrystalli]

Just to note that hubValidations does check that a file is is submitted to the correct directory, expecting it to match to the model ID parsed from the filename which itself expects the model_id format specified in the documentation. https://github.com/Infectious-Disease-Modeling-Hubs/hubValidations/blob/f6b9eb2fdda56b23f98e9214b8a4ec4dde6b1054/R/check_file_location.R#L9-L12
So it generally is a requirement for new hubs that use hubValidations.

hubData::connect_hub() is not as strict as it just populates the model_id information from the file structure, assuming it has been validated on the way in. So hubData::connect_hub() can work with historical data that does not fully conform to what we want (and is validated by hubValidations) in current hubs.

[elray1]

Thanks, @annakrystalli. I couldn't easily tell from the part of the code I looked at -- is there actually a check somewhere that the model_id has two parts separated by a -, or something along those lines?

[annakrystalli]

So that pattern (round_id-team_abbr-model_abbr) is required by parse_file_name() which expects that pattern to parse the filename into its components.

check_file_name() which is run prior to check_file_location() checks that parse_file_name() can be run without errors, which effectively checks the filename contains the correct model_id pattern. Then check_file_location() checks that the model id in the filename matches the folder the file is being submitted to, so inadvertently is checking that it matches the correct model id pattern too.

[annakrystalli]

Hmmm looking at it more closely, it's a bit more brittle what's going on. So parse_file_name() extract metadata from the filename by matching group patterns that are either dates or alphanumerics + _. Anything not matching such pattern would be considered a delimiter and ignored. The functions extracts all groups matching that pattern and counts to ensure the right number have been extracted and assigns each to the appropriate element of the output. It does however recontruct model_id by pasting team_abbr and model_abbr together using a - separator. So atleast if the model id iin the filename does not match that pattern, an error would be thrown by check_file_location(). That's not really where you'd want to know about this though, right? I think it could be useful to add the following additional checks:

• That separators of metadata in the filename are single - and that,
• if a date is matched, it can only match the round_id element of it.

The upside of this deep dive however means that I believe parse_file_name is already set up to accept round_ids other than dates (the regex pattern is an OR statement, not a strict sequential statement) so long as the conform to the pattern [a-z_0-9]+.

[annakrystalli]

On a side note, with respect to effects on data transforms in the cloud, while I find the feature of adding model and team information in the parquet transformation useful, I do feel it should be able to be turned off and up to hub administrators to choose whether they want it because it does change the output of connect_hub queries when dealing with local vs cloud hubs which I don't feel should be enforced with no option to turn off.