Added version_prompt parameter to robyn_refresh()

[JustStas]

Project Robyn

Currently the refresh() function selects the refresh version on its own. Here I added an additional bool param version_prompt that allows to pause the code and ask the user to input a specific refresh model name.

Type of change

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

• Locally on personal machine using custom dataset

Checklist:

• Fork the repo and create your branch from master.
• If you've added code that should be tested, add tests.
• If you've changed APIs, update the documentation.
• Ensure the test suite passes.
• Make sure your code lints.

[facebook-github-bot]

Hi @JustStas!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[laresbernardo]

Thanks for submitting a PR. I understand what you did here but how does asking the user for a model ID here actually help? How will the user select a model ID while the function is waiting for a readline input without digging deep into models inspection?

[JustStas]

@laresbernardo thanks for the comment.
By the moment when we ask for user input, the one-pagers and pareto csv files are already generated, so the user can take a look at them and make their choice. For example, I currently am testing a separate py script that does holdout validation of a model - I would use that on each of the refreshed models, check the holdout RMSE (or some other metric) and choose the appropriate refresh version based on that.
A better solution would probably be to separate refresh generation and selection (like it is currently implemented for the init model), but this commit seems like a quick way to add functionality without breaking anything :)
Please tell me if I have not answered your question

[JustStas]

Also, you can see that I kept the default behaviour as it was, just added the option to enable the prompt - I think that it should not cause any harm, but could be a good way to add some flexibility.

[laresbernardo]

Also, you can see that I kept the default behaviour as it was, just added the option to enable the prompt - I think that it should not cause any harm, but could be a good way to add some flexibility.

Yes, noticed and agree. Let me talk with the team and please, follow the steps of the Facebook CLA Check for the License Agreement so we can merge this and any following PR if/when approved.

[Tsurty]

I agree this would be a pretty useful and quick addition to the current robyn_refresh function, because otherwise it doesn't make sense to plot several different models if in the end Robyn will automatically pick one anyway.

[JustStas]

Also, you can see that I kept the default behaviour as it was, just added the option to enable the prompt - I think that it should not cause any harm, but could be a good way to add some flexibility.

Yes, noticed and agree. Let me talk with the team and please, follow the steps of the Facebook CLA Check for the License Agreement so we can merge this and any following PR if/when approved.

I signed the CLA, but the failed check is still here. Do I need to do anything else?

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[laresbernardo]

I signed the CLA, but the failed check is still here. Do I need to do anything else?

No. You can check that check's done now.

I agree this would be a pretty useful and quick addition to the current robyn_refresh function, because otherwise it doesn't make sense to plot several different models if in the end Robyn will automatically pick one anyway.

@Tsurty I think that's not an option given we'd have to select a different model ID for each run so a fixed parameter doesn't make much sense. But feel free to change anything in this same branch before we proceed to merge.

Also, I want to comment that one of our main goals with Robyn is to minimize analyst bias and automate decisions, but also are open to enabling advanced parameters to advanced users in cases like this when required.

[laresbernardo]

Pending: add new parameter into function's documentation (using roxygen's format) and run devtools::document() to build .Rd files.

[JustStas]

Pending: add new parameter into function's documentation (using roxygen's format) and run devtools::document() to build .Rd files.

Done.

[laresbernardo]

@Tsurty I think that's not an option given we'd have to select a different model ID for each run so a fixed parameter doesn't make much sense. But feel free to change anything in this same branch before we proceed to merge.

Do you agree @Tsurty? Should we merge as is? Any improvement you'd like to provide or suggest to the flow suggested?

[Tsurty]

@Tsurty I think that's not an option given we'd have to select a different model ID for each run so a fixed parameter doesn't make much sense. But feel free to change anything in this same branch before we proceed to merge.

Do you agree @Tsurty? Should we merge as is? Any improvement you'd like to provide or suggest to the flow suggested?

I believe you are referring to my previous comment which I edited as it didn't really make sense indeed.
So in short: yes I agree with the current state of the modification and I've no improvement at the moment.