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 docstrings of aesaraf.join_non_shared_inputs #6105

Closed
ricardoV94 opened this issue Sep 7, 2022 · 3 comments · Fixed by #6216
Closed

Improve docstrings of aesaraf.join_non_shared_inputs #6105

ricardoV94 opened this issue Sep 7, 2022 · 3 comments · Fixed by #6216

Comments

@ricardoV94
Copy link
Member

ricardoV94 commented Sep 7, 2022

This function could benefit from some more clear docstrings and example.

  • Point is actually optional when make_shared is False
  • Explain shared
  • Make shared optional

pymc/pymc/aesaraf.py

Lines 565 to 586 in 761f77d

def join_nonshared_inputs(
point: Dict[str, np.ndarray],
xs: List[TensorVariable],
vars: List[TensorVariable],
shared,
make_shared: bool = False,
):
"""
Takes a list of Aesara Variables and joins their non shared inputs into a single input.
Parameters
----------
point: a sample point
xs: list of Aesara tensors
vars: list of variables to join
Returns
-------
tensors, inarray
tensors: list of same tensors but with inarray as input
inarray: vector of inputs
"""

@wd60622
Copy link
Contributor

wd60622 commented Oct 12, 2022

I am looking into this a bit but am currently understanding the function a bit more.

Is the point actually optional with make_shared = False?
I am seeing that point will be indexed in the for loop which seems like it will cause an error. Am I reading that wrong?

Seems like there would need some additional logic to make that happen.

@ricardoV94
Copy link
Member Author

@wd60622 You're right. In that case the error message in the ifelse branch makes no sense. We can just make the point a required argument and remove the message.

@wd60622
Copy link
Contributor

wd60622 commented Oct 12, 2022

@wd60622 You're right. In that case the error message in the ifelse branch makes no sense. We can just make the point a required argument and remove the message.

Gotcha. Also noticing that when this function is called (smc and metropolis files in helper functions), the make_shared is never set from the default. i.e. raised error never touch with the currently implemented pymc code. Where you suggesting removing the error raising? Is there justification for keeping that ifelse branch in all together.

Obviously different scope. I can at least take a stab at adding to the type hints, better documenting what currently exists, and try to make some example based on the code I see for the docstring

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

Successfully merging a pull request may close this issue.

2 participants