-
-
Notifications
You must be signed in to change notification settings - Fork 403
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
Update from_cmdstan converter to follow schema convention #1541
Update from_cmdstan converter to follow schema convention #1541
Conversation
Though this work could be done in other ways too, I decided not to edit the code much and do the work in least possible differences. |
Codecov Report
@@ Coverage Diff @@
## main #1541 +/- ##
==========================================
+ Coverage 91.07% 91.08% +0.01%
==========================================
Files 105 105
Lines 11361 11380 +19
==========================================
+ Hits 10347 10366 +19
Misses 1014 1014
Continue to review full report at Codecov.
|
arviz/data/io_cmdstan.py
Outdated
name = "diverging" if name == "divergent" else name | ||
name = "n_steps" if name == "n_leapfrog" else name | ||
name = "tree_depth" if name == "treedepth" else name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that now that multiple variables are renamed, using a dict as mapping like in io_numpyro
will be more clear
acc7535
to
080c32b
Compare
de27e3e
to
c02961e
Compare
sampler_params[j] = sampler_params[j].rename(columns=rename_dict) | ||
sampler_params_warmup[j] = sampler_params_warmup[j].rename(columns=rename_dict) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, we don't need these commands to rename the keys as renaming is already been taken care of in the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @utkarsh-maheshwari 😄 Can you add it to the changelog (together with your previous PR for which we skipped that step) before merging? Both PRs can be mentioned in a single bullet point with two pr links
c02961e
to
0c64002
Compare
Done! |
No problem, we have the checklist on the pr template because we all forget if we don't see it there. Thanks for the PR! |
…#1541) * Update from_cmdstan converter to follow schema convention * Updated CHANGELOG.md
Description
Matched sample stats according to InferenceData schema specification in from_cmdstan converter. Addreses issue #1514
Checklist