bugfix for fit_laplace absent dims #609
Conversation
…ms on data containers and deterministics
ricardoV94
left a comment
ricardoV94
left a comment
There was a problem hiding this comment.
why are we forcing dims in variables that didn't have then originally?
Hey @ricardoV94, I am not sure I understand what you mean here. I compared the returned |
pm.sample doesn't add dims to model variables. Those show up in the conversion to InferenceData by arviz |
…variables that did not have them originally
Thank you, @ricardoV94. I was not aware of that. I made a change so that we don't force any dims on variables that were not assigned any in the original model context. |
|
Hey @ricardoV94, anything I can add/modify here to move it forward? |
| laplace_model.add_coords( | ||
| {name: np.arange(shape) for name, shape in zip(dims[2:], dim_shapes)} | ||
| ) | ||
| if dim_shapes[0] is not None: |
There was a problem hiding this comment.
I don't follow what's hapening here. If dim_shapes is coming from rv.type.shape, you could have dim_shapes[0] not be None, and a follow up be None. Example x = pm.Data("x", np.zeros(5, 3), shape=(5, None) is valid.
The whole approach seems a bit backwards. Instead of trying to plug the sample dims in the model that may not have properly defined dims to begin with, why not sample first and then attach those in the final inference data object, which always has dims, explicit or automatic?
If you just want to patch for now be more exhaustive and check not any(d is None for d in dim_shapes)
There was a problem hiding this comment.
@ricardoV94 Thank you for taking the time to review my changes. I went back and looked at the logic and I agree with you that it would be more appropriate to sample and allow the inference data object to create dims automatically. However, due to the addition of (temp_chain, temp_draw) dims inmodel_to_laplace_approx the automatic dims are incremented by 2. For example, the expected mu_dim_0 would be mu_dim_2. I wrote a helper function to rename these automatically generated dims/coords post creation. Please let me know if that looks okay to you.
…s the dimension names after
2 participants
updated dim_shape assignment logic in fit_laplace to handle absent dims on data containers and deterministics. I also added a test for that specific scenario.
closes #581