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.

Sign up for GitHub

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

Jump to bottom

Check all calls to `as.data.table() to make sure they are not dropping classes we want to keep #587

Closed
nikosbosse opened this issue Jan 7, 2024 · 5 comments · Fixed by #802
Closed

Check all calls to `as.data.table() to make sure they are not dropping classes we want to keep #587

nikosbosse opened this issue Jan 7, 2024 · 5 comments · Fixed by #802
Labels
good first issue Good for newcomers package improvement
Milestone
scoringutils-2.0

Comments

@nikosbosse
Copy link
Contributor

nikosbosse commented Jan 7, 2024 •
edited

As described in #559, some functions cause an object to lose its forecast_* class by calling as.data.table(). We should go through all functions to make sure this isn't happening / not causing any issues.
This likely means creating a unit test for any function that calls as.data.table() to make sure it has the output class we expect it to have.
@nikosbosse nikosbosse mentioned this issue Jan 7, 2024
Merged
9 tasks
@nikosbosse nikosbosse added this to the scoringutils-2.0 milestone Jan 7, 2024
@seabbs
Copy link
Contributor

seabbs commented Feb 28, 2024

The work I see for this is:

  • Make a generic test to see if the input and output of a function has the same class
  • Make a list of functions to apply this to
  • Iterate over the list and run the test
  • Fix any cases where issues are found
  • Leave in test suite to make sure we have good coverage of this problem going forward.

@nikosbosse
Copy link
Contributor Author

Excellent!

@seabbs seabbs mentioned this issue Feb 28, 2024
Closed
@nikosbosse
Copy link
Contributor Author

Are you fine with me moving this to 2.1.?

@seabbs
Copy link
Contributor

seabbs commented Apr 8, 2024

No not really. I think all functions should be clearly testing their output return is the class that is expected and we really want that in place now vs later.

I am worried that this will cause people a lot of code errors when porting their code and therefore a lot of frustration.

@nikosbosse
Copy link
Contributor Author

Hm... I don't really know how to implement this generic test (given that all the functions have different inputs). I think I'm just going to iterate over all functions and a test for their output class...

@nikosbosse nikosbosse mentioned this issue May 5, 2024
Merged
9 tasks
nikosbosse added a commit that referenced this issue May 18, 2024
@nikosbosse
Merge pull request #802 from epiforecasts/add-tests-score
39ac584 
Issues #502 and #587- Add additional tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for newcomers package improvement
Projects
scoringutils 2.0
Status: Done
Milestone
scoringutils-2.0
Development

Successfully merging a pull request may close this issue.

Issues #502 and #587- Add additional tests epiforecasts/scoringutils
2 participants
@seabbs @nikosbosse