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

use chain serial rather than logical index with _DefaultTrace.insert #1590

Merged
merged 2 commits into from
Mar 3, 2021

Conversation

Spaak
Copy link
Contributor

@Spaak Spaak commented Mar 1, 2021

This is part of a fix for an issue in PyMC3: pymc-devs/pymc#4469 . A trace could have chains that are not necessarily indexed 0, 1, .... However, _DefaultTrace.insert does take an index 0, 1, ... So, the index argument into insert should be zero-based. This PR decouples the chain index (which refers to a logical chain index, possibly starting at 1 or whatever) from the index passed to _DefaultTrace.insert.

@OriolAbril
Copy link
Member

Looks good, thanks. It should probably be also added to pymc-devs/pymc#4489 better, which is where the converter will live in 4.0+

@Spaak
Copy link
Contributor Author

Spaak commented Mar 2, 2021

Thanks, I've pushed the change directly to the to_inference_data pymc3 branch as well.

@OriolAbril
Copy link
Member

Can you add it to maintenance and fixes in the changelog? I think it is good to merge after that

@Spaak
Copy link
Contributor Author

Spaak commented Mar 3, 2021

Yep, done! (Not sure you'd get a notification after a commit push? Hence this comment :) )

@OriolAbril OriolAbril merged commit c562fad into arviz-devs:main Mar 3, 2021
@OriolAbril
Copy link
Member

Thanks!

utkarsh-maheshwari pushed a commit to utkarsh-maheshwari/arviz that referenced this pull request May 27, 2021
…rviz-devs#1590)

* use chain serial rather than logical index with _DefaultTrace.insert

* add PR arviz-devs#1590 to changelog
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.

2 participants