-
-
Notifications
You must be signed in to change notification settings - Fork 396
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_xyz converters to follow schema conventions #1514
Comments
Hello community, I maybe wrong, but shouldn't "potential energy" be matched to "model_logp" and not "lp" because pyMC3 uses "model_logp" to depict potential energy according to this comment ? |
Hi, welcome! This screenshot is actually a very good example to illustrate the issue and the motivation behind establishing this sample stats naming convention. One of the goals of ArviZ is to be backend agnostic, thus, once converted to InferenceData, there should be no differences other than the attributes (where we store the date, libraries used to get the samples and versions...). However, each of the inference libraries uses its own naming convention, precisely as you have seen in the comment you mention above. Currently, some sample stats are already renamed to the ArviZ convention, but not all of them. This issue is to make sure we enforce the sample stats names defined in the schema. Now to the example at hand. Before agreeing on the names we just added to the schema, As for you question, to have the comment be exact yes, |
If no one is working on |
That would be great, thanks! |
Done. submitted my first pull request in any Organization. Thanks a lot @OriolAbril for helping with understanding the issue. |
I think nothing need to be changed in |
That is great then! Updating the issue description
Group names should already be right for all converters, except a couple hiccups in tfp case, see #812. Missmatches there would be a clear bug. |
I see. I would like to start updating |
Great, thanks! |
Hi @OriolAbril, I would like to work on this issue. I followed the conversation. I would like to know what's the remaining part to be done and kindly provide me some information towards the same. If there is any .py file I need to work on specifically etc, Please guide me through it. |
Hi @madhucharan! @utkarsh-maheshwari has already worked on a couple converters, I have linked the PRs in the description above but there are still some converters to edit (also edited the description to show that). Let me know which would you like to work on and I can give some extra specific guidance if needed. |
Hi @OriolAbril , I would like to work on pymc3 or pystan.Could you please guide me further? Meanwhile I will look at the previous changes to numpyro and others to get some sense of it. |
I would recommend starting with PyMC3, which will be quite similar to numpyro. There already is a rename dict defined at https://github.com/arviz-devs/arviz/blob/main/arviz/data/io_pymc3.py#L277 which has to be updated to have all the renamed defined in #1063 (comment). PyStan will be somewhat similar to cmdstan, and will be a bit more convoluted. I think there are 3 places Lines 651 to 667 in 6fa1ce8
Lines 782 to 793 in 6fa1ce8
|
Thank you for the clear explanation @OriolAbril .Will get started with my first contribution to get familiar with the codebase. Will reach out after working on pymc3 |
Can I start updating |
Updated the description, now only from_pystan is unassigned, thanks :) |
Hi @OriolAbril , I have changed the pymc3 rename dict and I tried a lot today to rebase the repo to sync my fork, but unable to do so Not sure why.
getting this error.Could you please guide me how to fix this? or should I send a pr regardless of this but I think I will need to sync my fork with main repo in long run |
Hi @madhucharan https://docs.github.com/en/github/authenticating-to-github/generating-a-new-ssh-key-and-adding-it-to-the-ssh-agent : Generating a new SSH key and adding it to the ssh-agent. https://docs.github.com/en/github/authenticating-to-github/adding-a-new-ssh-key-to-your-github-account : Adding a new SSH key to your GitHub account. |
I will be updating from_cmdstan
Thank you @utkarsh-maheshwari. @OriolAbril I resolved it by removing the git URL to HTTP URL of base repository. Is it fine to do so or do we need to follow above steps itself? |
@OriolAbril , I have sent a PR for pymc3, please review. I would like to update pystan as well. Shall I start working on it? |
There are multiple ways to authenticate, if you are now able to work without issues that is fine, no need for extra changes.
Okey, I will update the description |
I guess this issue can be closed now. @OriolAbril ? |
yeah, let's keep track of everything tfp related in #812 |
We recently merged #1063 after deciding on the "ArviZ name" for each of the parameters.
Thoughts on implementation
Some of the libraries already rename some parameters, in many cases it will only mean adding some extra mappings to what is already happening. See for example https://github.com/arviz-devs/arviz/blob/master/arviz/data/io_numpyro.py#L136-L141.
Feel free to work only on one or several converters, but please comment below which are you targetting.
The text was updated successfully, but these errors were encountered: