-
Notifications
You must be signed in to change notification settings - Fork 364
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
Robyn Refresh error #960
Comments
There are a couple of bugs I've found in refresh functionality and am working on fixes on my end. Can you try updating Robyn with |
your second screenshot might be caused by too few iterations for refresh. can you increase refresh iters and retry? For the other errors, I'm waiting for the result from Bernardo's fixes first. |
@shuvayan @bart-vanvlerken we are planning on landing a CRAN stable version this week. Please, let us know asap if the issue persists with the version of the branch I shared so we can move forward or hold it. |
@laresbernardo @gufengzhou I'm going to try Bernardo's latest version today and will let you know! Thanks for all the help. |
Thanks for confirming @bart-vanvlerken. So these are all warnings, not errors, which is good because you can refresh the model. about the warnings:
By default, Robyn creates one-pagers for the lowest combined errors models per cluster. The default selected model on refresh is the one with lowest DECOMP.RSSD error (NRMSE not used in the criteria) which not necessarily will match. We could improve this behavior on following versions though, but it's not exactly an error. You can recreate any one-pager with About the file, did you check for the CSV file created and shown in the log in that specific folder? Wasn't it created? |
Hi @laresbernardo, you're right that they are warnings and not errors. However, I did specify ts_validation = TRUE in my base model (and the exported JSON). So I think it's strange that robyn_refresh() does not recognize this, when in the current CRAN version this is not an issue. Regarding the use of robyn_csv() instead of robyn_onepagers(), that was my bad! Thank you for clearing that up :) Regarding the exported onepagers in the refresh, it exported four onepagers and the optimal onepager (which was also exported as CSV) was not one of them. |
Alright @bart-vanvlerken would you mind updating to my branch once again and retrying? I've just deployed a fix to the On the one-pagers, as I mentioned before:
I'll try to add the selected refresh model to the list of exported one-pagers in addition to the clustering soon, and will let you know in this thread. |
Hi @laresbernardo, I updated and tested. The ts_validation issue is now resolved! However, the 'optimal' model chosen by the refresh function is still not exported as a onepager. I wanted to recreate this myself since the JSON file was exported correctly. However, when running robyn_recreate() I'm running into the following error: |
…folder + penalty hyps allowed #960
Alright @bart-vanvlerken, would you mind updating Robyn to my branch (bl01) and retrying? You should get the one-pager for the winning refresh model created by default. And also, probably have fixed the "penalty" hyperparameters check issue. Please confirm. |
Hello @laresbernardo , I am getting the below error after updating to your branch: Recreating model
I am using the below code for model refresh:
|
@shuvayan Can't replicate your issue. Maybe it's related to the impressions variables. Would you mind sharing your mmm_input_ufpf CSV and the json file via email to me? It's my user @ gmail.com. @bart-vanvlerken is it running OK for you? |
@laresbernardo , I have shared the email, please check! |
Hi @laresbernardo, I used your latest version and the one-pager is exported correctly. However, I'm still not able to reproduce the model with the exported JSON. It gives me the following error: In addition, I question the decision to optimize refreshing models based on DECOMP.RSSD since it has a poor impact on model accuracy as you can see below. Finally, I see quite a discrepancy in ROIs between the base model... |
@shuvayan it seems you're trying to use a JSON file that is NOT a model. RobynModel-models.json is not a valid JSON file to recreate a model but the whole iterations process. Let me check though if I can replicate the issue..
@bart-vanvlerken would you mind opening new threads to discuss these? I may agree with you, but I'd like for @gufengzhou to explain why he decided to change from the combined minimum error to only DECOMP.RSSD and provide some more context. Also, if you'd like to share with me the model's JSON file and CSV to replicate the issues, please send it so I can try and replicate your issue with robyn_recreate(). |
Hi @bart-vanvlerken , I've just tested refresh with 4 new datapoints. It looks better than what you showed. You only used 200 refresh_iters, right? That's probably not enough. I used 1k. Regarding decomposition, please look at report_decomposition.png, not the default onepager. Regarding objectives, refresh still uses both NRMSE and DECOMP.RSSD to optimise. Only the final automated winner selection will rely on decomp, because the challenge of refresh is often the too big changes in decomp. |
Hi @gufengzhou, that is odd. In the screenshots above I actually used 3 x 2000 iterations so that convergence was not an issue. However, it still did not converge, perhaps because I based my refresh off of a calibrated model (so MAPE was active as well). Could that also be the reason we're seeing such big differences in model fit? I only see the left [0] visualization in my report_decomposition.png file for some reason. @laresbernardo Here is my JSON file, it's basically a configuration that aims to test most functionalities that Robyn has to offer. And the corresponding CSVs, basically the data that comes with the Robyn package: |
@bart-vanvlerken I can't seem to be able to reproduce you issue recreating a model. As you can see, with your JSON and CSV I was able to recreate the model with the latest version in branch "bl01":
|
@laresbernardo I'm sorry, I shared the base model with you (which works fine for me as well). Could you try to reproduce the refreshed model instead? That's what gave me the error. Here is the JSON: |
Alright, now I was able to find the problem and fixed it. Thanks. |
@laresbernardo that works, thanks for the fix! |
I guess in this thread the only pending issue is:
@gufengzhou would you mind checking this one? Are you able to reproduce? Is it because it doesn't find the original models maybe? Perhaps we should include all past models' information in refreshed models (JSON) instead of the model IDs / chain? I'd leave this improvement as a backlog task for now. |
Changes are ready to land in main branch. For the |
Landed to main: v3.10.7. Please update and retest. If no issues are reported in a couple of weeks we will land this version as the latest stable version in CRAN. Thanks for the feedback @bart-vanvlerken @shuvayan |
Hi @laresbernardo, I just tested refreshing a model on 3.10.7 and I'm getting the following error: Here is my data so you can reproduce: |
Hi @bart-vanvlerken thanks for sharing your files. I was able to replicate the error and have fixed the issue on my end. Would you mind testing with branch "bl02" now and confirming if it's running as expected for you? Update |
Hi @laresbernardo the refresh functionality works, but even with one new observation of data it's generating completely different ROAS figures than the base model: |
Hi, regarding the interpretation of refresh, the second plot is related to the file report_aggregated.csv. The concept behind refresh is that it should maintain a stable baseline compared to initial model, but reflect the changes in the new data. You should use report_aggregated.csv and report_decomposition.png to "report" the refresh results. Let's say you add 4 weeks, then refresh will try to find the best fit and smaller decomp error for the added 4 weeks, while keeping the refresh baseline similar to initial model. It means, in the second plot above, the 2_128_7 [0] is from the initial model (initial modeling window) and 1_450_5 [1] is from the 4 refresh weeks. The "normal onepagers" are always referring to the entire modelling window that is not exactly relevant for refresh. Hope it makes sense. |
Hi @gufengzhou, thanks for your swift reply! There are two things confusing to me that I hope you can clear up:
|
Hi, I just spent some time to look through the code base and checked a refresh case. I'm using the latest GitHub version. So far, the ROAS of each model are identical in the onepagers & the report decomp png. Your result definitely doesn't look right. What caught my eye is that in this comment from you, the report_decomposition.png plot shows the initial model 2_128_7 [0] on the right side, the 1st refresh on the left side. This shouldn't be the case. Not sure what caused this for you. You can see in my example below that the initial model is always on the left side. Maybe you can test run a quick new one with the latest version (no need for full iters), then refresh it to see if this is still the case? However, you're right about one thing. The ROAS of the 1st refresh (1_71_1 in my case) of the report_decomposition.png shouldn't be identical as its onepager. Now both numbers are the ROAS of the media across entire refresh modeling window, while we actually want to have the ROAS for new periods only for report_decomposition.png. We'll see this gets fixed. Also FYI, compared to ROAS, the effect share (or decomposition) of 1_71_1 in report_decomposition.png is indeed reporting the new period, NOT the entire refresh modeling window. |
Hi @gufengzhou, great that you will work on a fix! How strange that your ROAS figures do correspond and mine don't! I tried again using the latest Github main branch and the visualization issue also persists unfortunately. Here is the model and (demo) data used to come to my findings (note that clean_data_refresh is the full dt_simulated_weekly dataset, where I used a trimmed version to build the base model). In addition, I'm getting the following output at the end of the refresh that might have something to do with it: |
I found the issue. You're using both ts_validation = TRUE and add_penalty = TRUE, and there's a bug that doesn't pick up the train_size and the penalties when recreating the model. I've reran a job with ts_validation and penalty both true, then exported json and recreated it using the following code. Now the results are identical, which was not the case before.
Can you please check? It's on the branch |
Hi @gufengzhou, I've updated to the branch you mentioned and I noticed that the refresh function does not work on recreated models: when I use the JSON file generated by recreating the base model (as done in the demo script) it gives me the following error: Unfortunately the main issue is not solved for me either - in report_decomposition.png the visualizations are still twisted and the ROAS figures do not correspond with the one-page of the base model. Could it be something else? |
This is very strange. I've just tested robyn_refresh again and it works. See my screenshot. Also the report_decomposition.png looks correct. Are you 100% certain that you've got the right branch? Also, can you verify if this script gives you the identical result? I also see that you're having the "Must provide 'hyperparameters' in robyn_inputs..." error again. Not sure why I don't have it. I do notice that whenever I'm using your json to test robyn_recreate, I don't get identical decomp between json and recreated model. But when I use jsons exported from latest package version, I get identical results. I think if you can get identical results from the script mentioned above, then we're one step closer. |
@gufengzhou I will try your solution, in the meanwhile I'd like to send you my R code so you can try to reproduce - can I mail it to you? |
sure. gufeng@meta.com |
@gufengzhou I'm positive i've used your version, as you can see below: Your script gives me identical results, but I'm afraid we are comparing different things: you are comparing the base model JSON with the recreated model, while I was comparing the base model JSON with the recreated base model JSON. In order to produce the recreated base model JSON, I used the alternative approach that's mentioned in the demo script (since robyn_recreate() does not do so automatically): Have you had any luck reproducing my errors with the code I sent you over email? I hope we can resolve this! |
Alright...you're obviously a very thorough person:) You're looking at differences at the 5th digits. I'm gonna let this one count as matched. There're just several rounding steps that I don't even remember that might be the reason for this discrepancy. But I'm quite certain this is acceptable for vast majority of use cases. I did another comparison across original object, original json, recreated object and recreated json. I'd count them as identical. I'll check your refresh error bit later. |
That would be great, because a client is not able to refresh their model currently (on the CRAN version nor the dev version). I've sent you the data + JSON over mail so you can inspect at your convenience! |
After a quick check, I can't recreate the same model result from your json from the email (before we get to the refersh issue). I noticed that the Json is created with a previous version, so I kind of suspect this is the cause. Would you please try to remodel it with the latest version, select a comparable candidate as your old one, then use the new Json to check recreate & refresh? Please understand that this package is still undergoing constant development, so backwards compatibility is not always possible, even tho we try. |
I was under the impression that the CRAN releases would be stable enough, but I'll version-lock the package for my new clients from now on. I solved the issue by tweaking the JSON so that it matches the JSON content in the current exports. |
… upper constraints fix when higher than mean (#974) * fix: refresh hyps check #960 + use data available in json * fix: update based on gz's comments * fix: fixed penalties and other fixed hyps on refreshing models * fix: refresh plot when chain is broken + feat: new bounds_freedom parameter to overwrite default calculation * fix: import and store original model when not in original plot_dir * recode: applied styler::tidyverse_style() to clean code for CRAN * fix: paid_media_total calc * fix: print ExportedModel only when available * fix: deal with negative trend - negative trend is not interpretable for MMM - force negative coef when trend is negative to get positive decomp * fix: upper constraint issue on BA for target_efficiency and weibull adstock feat: instead of Inf, use channel_constr_up, which by default is 10 for target_efficiency * fix: reverse wrong bounds update in refresh_hyps The refactoring of initBounds & listOutputPrev in refresh_hyps was wrong in 774c18d * recode: apply styler::tidyverse_style() --------- Co-authored-by: gufengzhou <gufengzhou@gmail.com>
@bart-vanvlerken we've updated Robyn with a few improvements on refresh. Would you mind checking if it runs OK for you? We are going to update CRAN version later this week and would appreciate your feedback before we do. Thanks! |
* build(deps): bump braces from 3.0.2 to 3.0.3 in /website (#997) * fix: refresh hyps check + use data available in json + refresh hyps + upper constraints fix when higher than mean (#974) * fix: refresh hyps check #960 + use data available in json * fix: update based on gz's comments * fix: fixed penalties and other fixed hyps on refreshing models * fix: refresh plot when chain is broken + feat: new bounds_freedom parameter to overwrite default calculation * fix: import and store original model when not in original plot_dir * recode: applied styler::tidyverse_style() to clean code for CRAN * fix: paid_media_total calc * fix: print ExportedModel only when available * fix: deal with negative trend - negative trend is not interpretable for MMM - force negative coef when trend is negative to get positive decomp * fix: upper constraint issue on BA for target_efficiency and weibull adstock feat: instead of Inf, use channel_constr_up, which by default is 10 for target_efficiency * fix: reverse wrong bounds update in refresh_hyps The refactoring of initBounds & listOutputPrev in refresh_hyps was wrong in 774c18d * recode: apply styler::tidyverse_style() --------- Co-authored-by: gufengzhou <gufengzhou@gmail.com> --------- Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: gufengzhou <gufengzhou@gmail.com>
Hi all, hope you're doing well. I've found a bug in the latest github version of Robyn where I can't refresh my model. In addition, the output tells me 'Provided train_size but ts_validation = FALSE. Time series validation inactive.' when I set ts_validation = TRUE in my base model. Hope anyone can help!
The text was updated successfully, but these errors were encountered: