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

Create a struct for VariableSpace #10

Closed
mohamed82008 opened this issue May 5, 2019 · 5 comments
Closed

Create a struct for VariableSpace #10

mohamed82008 opened this issue May 5, 2019 · 5 comments

Comments

@mohamed82008
Copy link
Member

It would be nice to create a new struct VariableSpace and use that instead of Set for Sampler space. We can then overload Base.in to check if vn is inside space including the case when space is empty. We can overload in now with space::Set but that can cause confusion with the empty space case.

@mohamed82008 mohamed82008 transferred this issue from TuringLang/Turing.jl Dec 25, 2019
@phipsgabler
Copy link
Member

In my VarName refactoring PR #49, the overloaded in(::VarName, ::Tuple) is replaced by a new function inspace. Does that make this issue obsolete?

@mohamed82008
Copy link
Member Author

Maybe but I still prefer VariableSpace and Base.in, less new function names. This is one thing we need to consider in DPPL and VarInfo. There are too many similar function names that can probably be simplified with structs and dispatch on Base functions that make sense.

@yebai
Copy link
Member

yebai commented Apr 3, 2020

Maybe but I still prefer VariableSpace and Base.in, less new function names. This is one thing we need to consider in DPPL and VarInfo. There are too many similar function names that can probably be simplified with structs and dispatch on Base functions that make sense.

Mohamed has some valid points here. We'd like to reduce the number of APIs in VarInfo if possible.

@devmotion
Copy link
Member

I agree, having a VariableSpace struct might make sense due to the discrepancy of variable spaces to regular tuples caused by the empty spaces. But if we have a VariableSpace, one would probably have to add it to the API instead of inspace and one loses all existing functionality that is provided for free for tuples 🤷‍♂️

@yebai
Copy link
Member

yebai commented Apr 13, 2020

Seems this issue got fixed by #49?

@yebai yebai closed this as completed Dec 16, 2021
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

No branches or pull requests

4 participants