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

Replacing MyDOWEffect with TransformedRandomVariable in day of the week tutorial #402

Closed
gvegayon opened this issue Aug 21, 2024 · 5 comments · Fixed by #406
Closed

Replacing MyDOWEffect with TransformedRandomVariable in day of the week tutorial #402

gvegayon opened this issue Aug 21, 2024 · 5 comments · Fixed by #406

Comments

@gvegayon
Copy link
Member

Looks good. I think we just want to wait for #387 so we can avoid having to define MyDOWEffect.

We could also merge as is and address that later. Your call @gvegayon.

I'll wait

Originally posted by @gvegayon in #344 (comment)

@gvegayon
Copy link
Member Author

gvegayon commented Aug 21, 2024

I am replacing my personalized RV in the day-of-the-week tutorial with a TransformedRandomVariable, and I see one issue: The option to record the transformed variable is accessible during the sample call. I think this should be changed to specify the record parameter during construction instead. Otherwise, following the current implementation, I would have to modify the body of the sample function of the latent.HospitalAdmissions RV to record the transformed variable.

attn @damonbayer @dylanhmorris

@damonbayer
Copy link
Collaborator

I agree that record should be specified at construction.

@damonbayer
Copy link
Collaborator

On further reflection, I think we could do away with record and have optional names. If there is no name, the variable is not recorded. If a same is provided, the RandomVariable is always recorded. This avoids any scenario where a name is supplied but not used.

@dylanhmorris
Copy link
Collaborator

I think this is an intriguing proposal but deserves its own issue for discussion.

@gvegayon
Copy link
Member Author

For the moment, in the PR attached to this issue, I will pass record = True to latent.HospitalAdmissions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants