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

Update from_cmdstan converter to follow schema convention #1541

Merged
merged 2 commits into from
Feb 9, 2021

Conversation

utkarsh-maheshwari
Copy link
Contributor

@utkarsh-maheshwari utkarsh-maheshwari commented Feb 6, 2021

Description

Matched sample stats according to InferenceData schema specification in from_cmdstan converter. Addreses issue #1514

Checklist

  • Follows official PR format
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

@utkarsh-maheshwari
Copy link
Contributor Author

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.
Is this approach right?

@codecov
Copy link

codecov bot commented Feb 6, 2021

Codecov Report

Merging #1541 (601eb76) into main (6fa1ce8) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
arviz/data/io_cmdstan.py 91.89% <100.00%> (-0.08%) ⬇️
arviz/stats/diagnostics.py 99.26% <0.00%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fa1ce8...0c64002. Read the comment docs.

Comment on lines 214 to 216
name = "diverging" if name == "divergent" else name
name = "n_steps" if name == "n_leapfrog" else name
name = "tree_depth" if name == "treedepth" else name
Copy link
Member

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

@utkarsh-maheshwari utkarsh-maheshwari force-pushed the update-io_cmdstan branch 3 times, most recently from acc7535 to 080c32b Compare February 7, 2021 20:08
arviz/data/io_cmdstan.py Show resolved Hide resolved
@utkarsh-maheshwari utkarsh-maheshwari force-pushed the update-io_cmdstan branch 3 times, most recently from de27e3e to c02961e Compare February 8, 2021 09:55
Comment on lines -220 to -221
sampler_params[j] = sampler_params[j].rename(columns=rename_dict)
sampler_params_warmup[j] = sampler_params_warmup[j].rename(columns=rename_dict)
Copy link
Contributor Author

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.

Copy link
Member

@OriolAbril OriolAbril left a 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

@utkarsh-maheshwari
Copy link
Contributor Author

Done!
Sorry, forgot to add it earlier.

@OriolAbril OriolAbril merged commit 78ec5a0 into arviz-devs:main Feb 9, 2021
@OriolAbril
Copy link
Member

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!

@utkarsh-maheshwari utkarsh-maheshwari deleted the update-io_cmdstan branch May 27, 2021 09:38
utkarsh-maheshwari added a commit to utkarsh-maheshwari/arviz that referenced this pull request May 27, 2021
…#1541)

* Update from_cmdstan converter to follow schema convention

* Updated CHANGELOG.md
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

Successfully merging this pull request may close these issues.

3 participants