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

Logical error in selecting the best model for Robyn_refresh #674

Closed
sahbakn opened this issue Apr 4, 2023 · 3 comments
Closed

Logical error in selecting the best model for Robyn_refresh #674

sahbakn opened this issue Apr 4, 2023 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@sahbakn
Copy link

sahbakn commented Apr 4, 2023

Describe issue

I believe there might be a logical error is selecting the best model based on 'error_scores' in refresh.R.
Based on my understanding, we want to choose the model with the highest error score. However, the code snippet below shows that the sorting is done on 'solID' first and then 'error_score', and given the solIDs are unique, the selection is done on the string of solIDs not on error score. I have seen this in practice too, seeing the next model for refresh is selected based on the solIDs strings.

In Refresh.R:

    ## Select winner model for current refresh
    OutputCollectRF$resultHypParam <- OutputCollectRF$resultHypParam %>%
      arrange(.data$solID, desc(.data$error_score)) %>%
      select(.data$solID, everything()) %>%
      ungroup()
    bestMod <- OutputCollectRF$resultHypParam$solID[1]

Environment & Robyn version

Robyn version 3.7 and above

@sahbakn sahbakn changed the title Selecting the best model for Refresh Logical error in selecting the best model for Robyn_refresh Apr 4, 2023
@laresbernardo laresbernardo self-assigned this Apr 12, 2023
@laresbernardo laresbernardo added bug Something isn't working and removed bug Something isn't working labels Apr 12, 2023
@laresbernardo
Copy link
Collaborator

Actually, I think you're right! Let me double check and will get back to you. Thanks for reporting it @sahbakn

@laresbernardo
Copy link
Collaborator

I've just deployed a fix directly to main branch @sahbakn
Thanks again for double checking for us. Please update Robyn and re-run the refresh function to automatically get the max(errors_score) model by default (and not the first one based on IDs!).
Also, after validating it runs as expected, feel free to close this ticket.

@laresbernardo laresbernardo added the bug Something isn't working label Apr 12, 2023
@sahbakn
Copy link
Author

sahbakn commented Apr 13, 2023

Thanks @laresbernardo it is fixed now

@sahbakn sahbakn closed this as completed Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants