supported variable mapping in stan convertors#163
Conversation
| def posterior_predictive_to_xarray(self): | ||
| """Convert posterior_predictive samples to xarray.""" | ||
| data, data_warmup = self.predictive_to_xarray(self.posterior_predictive, self.posterior) | ||
| posterior_predictive = self.posterior_predictive |
There was a problem hiding this comment.
I mentioned observed_data, log_likelihood and posterior_predictive in my comment, but then also mentioned to look at behaviour differences between log_likelihood and posterior_predictive or predictions. Renames might be needed for log_likelihood, posterior_predictive, predictions and/or prior_predictive.
Tangentially, the converter has this predictive_to_xarray function that is called in both posterior_predictive and prior_predictive because the logic for both is extremely similar. We are far from perfect (in fact, predictive_to_xarray could also be used for predictions and it is not, but we try to keep things simple and maintainable. If after some changes you end up with a function that is only ever called once by another one it is a signal for asking about it and maybe re-evaluating the need for that function (it might still abstract some logic away to keep the second function easier to follow).
| if isinstance(posterior_predictive, str): | ||
| posterior_predictive = [posterior_predictive] | ||
|
|
||
| if isinstance(posterior_predictive, list | tuple): | ||
| posterior_predictive = {name: name for name in posterior_predictive} | ||
|
|
||
| data, data_warmup = _unpack_fit( | ||
| self.posterior, | ||
| list(posterior_predictive.values()), | ||
| self.save_warmup, | ||
| self.dtypes, | ||
| ) | ||
|
|
||
| data = { | ||
| obs_var_name: data[var_name] | ||
| for obs_var_name, var_name in posterior_predictive.items() | ||
| if var_name in data | ||
| } | ||
|
|
||
| if data_warmup: | ||
| data_warmup = { | ||
| obs_var_name: data_warmup[var_name] | ||
| for obs_var_name, var_name in posterior_predictive.items() | ||
| if var_name in data_warmup | ||
| } |
There was a problem hiding this comment.
In a similar vein to the previous comment. If log_likelihood_to_xarray is already doing the renames we want why develop a whole new logic to the renaming? It means the next person to work on this file will need to understand two different sets of code that are actually doing the same, making their work harder (also that next person could be future you or future me).
Overall, in this particular case, my recommendation is to use the same logic as in log_likelihood but adding it within predictive_to_xarray which will then be called from posterior_predictive, prior_predictive (both already do) and predictions (doesn't yet).
| for obs_var_name, log_like_name in log_likelihood.items() | ||
| if log_like_name in log_likelihood_draws_warmup | ||
| } | ||
| if ( |
There was a problem hiding this comment.
I like the idea of renaming automatically whenever there is only one observation and log_likelihood is a string. That being said, I think this is something that needs a bit more thinking. If we do it for the log likelihood why not do it also for the posterior predictive? And if we do it in multiple places, then it is probably better to define the obs_name in the __init__ as a special attribute then in the first check for log likelihood being a string, updating log likelihood to either a list or a dict depending on this obs_name attribute being present (and similar idea for posterior_predictive...).
| posterior, | ||
| model=posterior_model, | ||
| variables=posterior_predictive, | ||
| variables=list(posterior_predictive.values()), |
There was a problem hiding this comment.
given we'll want these renames to happen in several groups and we are basically using posterior_predictive, data and data_warmup I think it will be helpful to allow variables to be a dictionary and if that is the case handle the renaming directlly in get_draws
There was a problem hiding this comment.
test should be added yes but they should be added along the existing ones https://github.com/arviz-devs/arviz-base/tree/main/external_tests.
Also, in none of these 4 tests the name or docstring has anything to do with the actual body. The name makes it sound it is testing these new features but that is not the case at all. Please review your own changes and ask questions, you can review your own PR too to ask questions about specific parts of your proposed changes.
OriolAbril
left a comment
OriolAbril
left a comment
There was a problem hiding this comment.
in the previous round of review I mentioned the tests that were being added were not useful but I also said tests should be added and pointed out where they should go into. I am struggling to follow why there are now no tests at all nor any question related to that
| def posterior_predictive_to_xarray(self): | ||
| """Convert posterior_predictive samples to xarray.""" | ||
| data, data_warmup = self.predictive_to_xarray(self.posterior_predictive, self.posterior) | ||
| data, data_warmup = self.predictive_to_xarray(self.prior_predictive, self.prior) |
There was a problem hiding this comment.
it seems very dumb but if you review the diff of your own PR you'll easily things like this yourself
Thanks for the clarification . |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #163 +/- ##
===========================================
- Coverage 72.30% 50.51% -21.80%
===========================================
Files 19 20 +1
Lines 1914 2057 +143
===========================================
- Hits 1384 1039 -345
- Misses 530 1018 +488 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@OriolAbril I have added a test file for both converters with required changes(not fully furnished yet). |
|
@OriolAbril Any update ? |
|
@OriolAbril Changes addressed as per the requirement . |
|
@OriolAbril Sorry for tagging you again but is there any update regarding this PR ? |
|
@OriolAbril Any update regarding this PR ? |
|
Any update regarding this PR? |
3 participants
#59
This PR updates the Stan converters to support mapping predictive and log-likelihood variables to the corresponding observed variable name.
Changes
All tests and pre-commit checks pass locally.