Draft: REF: Rename ar variables

[juanmpga]

Hi all,

going through Time_Series_Generative_Graph I found the naming of the variable ar_innov slightly confusing as innovations usually refers to the forecast error (the term pm.Normal.dist(sigma=sigma) within ar_step in this case) and following @jessegrabowski 's suggestion I'm proposing a renaming.

• ar_innov -> ar_steps

I've also renamed the ar function names to give them verb-like structure:

• ar_step -> get_next_step
• ar_dist -> get_ar_steps. In the cases of pm.CustomDist("ar_dist"...) I've replaced by "ar_steps" for consistency.
• conditional_ar_dist -> get_conditional_ar_steps
• ar_dist_forecasting -> get_ar_steps_forecast (I could've used forecast_ar_steps but it could introduce potential confusing given how adjacent variables are named)

and similarly some variables to give them noun-like strucutre:

• forecast_inital_state -> initial_state_forecast

I sometimes name ar_init as ar_0 and ar_steps as ar_t for compactness.

I haven't created an issue because I felt this was too trivial but I have no trouble creating one.

Disclaimer: this is my first potential contribution to an OS project (at all!), I've read the guides and hope I haven't missed anything important.

• Notebook follows style guide https://docs.pymc.io/en/latest/contributing/jupyter_style.html
• PR description contains a link to the relevant issue:
  • a tracker one for existing notebooks (tracker issues have the "tracker id" label)
  • or a proposal one for new notebooks
• Check the notebook is not excluded from any pre-commit check: https://github.com/pymc-devs/pymc-examples/blob/main/.pre-commit-config.yaml

---

📚 Documentation preview 📚: https://pymc-examples--766.org.readthedocs.build/en/766/

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[ricardoV94]

I don't like the get_ prefix. Doesn't really add more info? The _dist name was a conscious choice, because it's the function passed into the dist argument of CustomDist. It's the function that defines the generative graph of the distribution.

innovations -> steps sounds fine

[juanmpga]

Yes I sometimes get the same feeling with the get_ prefix although I also find it a convenient (short) way to turn nouns into verbs which can be useful to distinguish functions from variables. Perhaps this distinction is not relevant to you?
In that case I can revert to the original variable names except for the "innovations -> steps" change.
Otherwise some alternatives could be: calc_next_step, regress_next_step, autoregress_next_step, define_ar_dist, define_ar_forecast_dist. It's also true that these names can get quite long which can be quite annoying when trying to stay below 80 characters.

[ricardoV94]

In that case I can revert to the original variable names except for the "innovations -> steps" change.

That would be my suggestion

[juanmpga]

Done!

With respect to optimal naming conventions I love this quote: "There are only two hard things in Computer Science: cache invalidation and naming things". Do you have any references (or things you've written) that you like particularly on the topic?

[ricardoV94]

I agree it's hard. The other issue in an OSS is that it's subjective. So someone may come next and decide they don't like the current names, and propose a change. Then someone says they like something like the original ones, and so on...

So if we are not adding anything extra to the codebase/documentation I would generally suggest not changing anything unless it's clearly needed.

That's why tools like black are right to be opinionated: "Here is an automatically-enforced default, no more discussions about how to format code/etc".