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

Keep names but label in as_forecast? #908

Closed
seabbs opened this issue Sep 12, 2024 · 8 comments · Fixed by #915
Closed

Keep names but label in as_forecast? #908

seabbs opened this issue Sep 12, 2024 · 8 comments · Fixed by #915

Comments

@seabbs
Copy link
Contributor

seabbs commented Sep 12, 2024

Here we rename inputs into our standard format. Some users (especially maybe devs) may want to keep their own variable names in output (i.e. hubverse-org/hubEvals#46) whilst we may prefer a workflow with class converters to and from our format and others we might want to consider alternatives (i.e issue title is one alternative).

Personally, i think that this would be a fairly large refactor with fairly little gain but still its a maybe.

@seabbs
Copy link
Contributor Author

seabbs commented Sep 12, 2024

Kind of as an aside in this particular case we could also consider standarising on the ecosystem choice of model_id vs model as we do now.

@nikosbosse
Copy link
Contributor

We could secretly allow model_id as an alternative as well to solve that specific use case. Nobody needs to know!

Labels would require an attribute which is again a bit brittle... I think the restriction we have makes things a lot easier

@seabbs
Copy link
Contributor Author

seabbs commented Sep 16, 2024

We could secretly allow model_id as an alternative as well to solve that specific use case

Stop - you are making me cry. 👎🏻

Labels would require an attribute which is again a bit brittle

I really don't like this idea either but I'm not sure of alternative is pre and post transformers aren't a goer.

I do think there is a reasonable case that people have thought about model vs model_id and prefer model_id so maybe we should just use that and accept (another) breaking change. What we could do is support transforming model to model_id for a bit with a depreciation note?

@sbfnk
Copy link
Contributor

sbfnk commented Sep 16, 2024

Keeping track of column names is quite a pain in socialmixr. In fact I found the solution here so much cleaner that I've been thinking we should adopt the same approach there.

All that said I can see that we need predicted and observed but does the package really need to know which column identifies forecasters? Isn't it just one way of aggregating?

@seabbs
Copy link
Contributor Author

seabbs commented Sep 16, 2024

I agree if we can just do away with needing the model column that would be ideal. I think its just fairly hard coded in a bunch of places vs really really needing to be there?

@sbfnk
Copy link
Contributor

sbfnk commented Sep 16, 2024

I've just done a cursory full-text search and this seems like a legacy requirement to me.

@seabbs
Copy link
Contributor Author

seabbs commented Sep 16, 2024

I see it in two places (from this): https://github.com/search?q=repo:epiforecasts/scoringutils%20%22model%22&type=code

  • summarise_scores: Having some model id allows for a nice default setting when summarising. Its not totally clear to me what we could replace this with here (aside from no default)
  • compare_two_models: In permutation testing hard codes the right hand side to be model and the entire function is built on the idea that you have named models and those have an id. This could be retooled to take a ID (and it could potentially default to something (i.e model).

@nikosbosse
Copy link
Contributor

Open to changing it.

I think the old idea was really mostly: it's clean and simple. Also it's only soft-enforces: if you don't provide a model column, we'll just add one for you.

@sbfnk sbfnk mentioned this issue Sep 18, 2024
9 tasks
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 a pull request may close this issue.

3 participants