The Wayback Machine - https://web.archive.org/web/20201126183208/https://github.com/facebook/prophet/issues/1151
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

Improve coherence in function naming and documentation #1151

Closed
adrfantini opened this issue Sep 24, 2019 · 3 comments
Closed

Improve coherence in function naming and documentation #1151

adrfantini opened this issue Sep 24, 2019 · 3 comments

Comments

@adrfantini
Copy link
Contributor

@adrfantini adrfantini commented Sep 24, 2019

While Prophet is a very powerful library which produces excellent forecasting, its function structure and documentation quality is not at the same level.

For example, functions do not seem to follow a coherent naming scheme.
For instance, in R: some functions use _ as separator (add_country_holidays), some . (fit.prophet). Some plotting functions start with plot (plot_forecast_component), some do not (prophet_plot_components).

Personally I would suggest that this should be addressed sooner rather than later, when more people will use the library and change will be harder.

Additionally, always in R, the documentation is somewhat lacking:

  • Many functions are missing examples
  • Many arguments and functions are not well explained (e.g. cross_validation)

Some lack of coherence can be found in arguments type as well: add_coutry_holidays accepts an input like "US", while using prophet directly does not (at least according to the doc).

Hopefully this could be improved in future revisions!

@bletham
Copy link
Contributor

@bletham bletham commented Sep 28, 2019

Thanks for the examples, and I think there is definitely room for improvement in the documentation within the code. Some of the incoherence is due to trying to simultaneously follow R standards while also making the code as similar as possible to the Python version. That's the reason fit.prophet and predict.prophet use periods, but all the other functions use underscore. R requires the specific fit.{classname} and predict.{classname} function names in order to get S3 methods like fit(prophet_object) and predict(prophet_object). So they have to use the period, but we decided to use _ for all of the others so that the function names match exactly the name of the equivalent method/function in Python. But some of the other function names could certainly be made more clear, and of course docstrings improved.

@adrfantini
Copy link
Contributor Author

@adrfantini adrfantini commented Sep 30, 2019

Thank you for your reply. Of all of the above, I would think that documentation is the most pressing issue. There is a lot of info on the online manual which is missing in the R/python doc, as far as I can tell.

@bletham
Copy link
Contributor

@bletham bletham commented Sep 3, 2020

Docstrings were fixed in the latest version.

@bletham bletham closed this Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.