Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Consider renaming Deterministic to store_in_trace #6695

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

Closed
ricardoV94 opened this issue Apr 28, 2023 · 17 comments
Closed

Consider renaming Deterministic to store_in_trace #6695

ricardoV94 opened this issue Apr 28, 2023 · 17 comments

Comments

@ricardoV94
Copy link
Member

ricardoV94 commented Apr 28, 2023

Description

Deterministic does not convey the meaning of what it does. It stores an arbitrary computation (including stochastic nodes) in the trace, between sampled points.

From experience with Discourse users (most recent example), I think Deterministic is a source of confusion. Some users feel they need it for everything that is not a pm.Distribution. This is

  1. Not true,
  2. A potential waste of computation and slowdown
  3. Can make the model API seem quite "uglier/verbose" than it actually is.

We should keep the Deterministic as an alias for quite some time, but start using the new name in docs and examples.

Alternative names welcome

@ricardoV94 ricardoV94 changed the title Rename Deterministic to store_in_trace Consider renaming Deterministic to store_in_trace Apr 28, 2023
@cuchoi
Copy link
Member

cuchoi commented May 15, 2023

That makes a lot of sense to me... are there any other uses for pm.Deterministic? We could put a deprecation warning on pm.Deterministic, but that might be a bit noisy given its adoption level.

@ricardoV94
Copy link
Member Author

ricardoV94 commented May 15, 2023

Storing in trace and showing in graphviz are the only uses I can think of.

@fonnesbeck
Copy link
Member

Perhaps something like store_values? If you are using VI, there will not be a trace per se, so may as well be agnostic there.

@ricardoV94
Copy link
Member Author

Short would also be nice, maybe something like record?

@fonnesbeck
Copy link
Member

fonnesbeck commented May 15, 2023

Was also thinking about capitalization/camel case here. I assume we will also need to pass a name, so might be odd to have these using a lower case/underscore convention while everything else in the model is camel cased. The camel case convention sort of implies that it is a first-class variable in the model, which I like.

theta = store_values('theta', alpha + beta * X) or theta = record('theta', alpha + beta * X)

versus

theta = StoreValues('theta', alpha + beta * X) or theta = Record('theta', alpha + beta * X)

@fonnesbeck
Copy link
Member

Alternative names:

  • Memoize
  • Track
  • Register

feel free to add others if anyone has ideas.

@ricardoV94
Copy link
Member Author

Not Register, because we "register" all variables. Others sound good.

Does github have polls?

@fonnesbeck
Copy link
Member

Here's a quick poll. Use the associated emoji to vote.

  • Memoize 🎉
  • Track ❤️
  • Store 🚀
  • Record 😄
  • none of the above 👎

@twiecki
Copy link
Member

twiecki commented May 16, 2023

image

@fonnesbeck
Copy link
Member

Its a virtual tie internally, so let's go with Record.

@twiecki
Copy link
Member

twiecki commented May 22, 2023

Final results
image

@ricardoV94
Copy link
Member Author

ricardoV94 commented Aug 11, 2023

This user was getting OOM problems because of the behavior of Deterministic: #6852

OTOH calling something Deterministic sounds nice for targeting by the do and observe functionality that will soon move from pymc-experimental to pymc. That makes me a bit reluctant about the renaming

@twiecki
Copy link
Member

twiecki commented Aug 11, 2023

I don't follow.

@ricardoV94
Copy link
Member Author

ricardoV94 commented Aug 11, 2023

with pm.Model() as m:
  ...
  x = pm.Deterministic("x”, ...)
  ...
new_m = observe(m, {x: data})
new_m = do(m, {x: intervention})

Is more intuitive when x was defined as a "Deterministic" vs as a "store_in_trace".

@twiecki
Copy link
Member

twiecki commented Oct 10, 2023

with pm.Model() as m:
  ...
  x = pm.Deterministic("x”, ...)
  ...
new_m = observe(m, {x: data})
new_m = do(m, {x: intervention})

Is more intuitive when x was defined as a "Deterministic" vs as a "store_in_trace".

I don't feel the same holds quite true for Record(). Another way would be NamedDeterministic()/NamedDet(), or StoredDeterministic()/StoredDet().

@trendelkampschroer
Copy link

trendelkampschroer commented Mar 22, 2024

Thanks for the great package and making it freely available. It's a pleasure learning Beyesian data analysis through the many great examples and pymcs modeling language.

In addition emphasising the potential caveats of using pm.Deterministic in the examples, e.g here would be helpful to new users.

Especially for hierarchical models, cf. here lead me to include "deterministics" into models and end up with unnecessarily large traces and compute times for e.g. arviz.summary(trace).

Also consider the usage of 'deterministics' here which might also be confusing - at least when starting with pymc.

@pymc-devs pymc-devs locked and limited conversation to collaborators Mar 22, 2024
@ricardoV94 ricardoV94 converted this issue into discussion #7209 Mar 22, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests

5 participants