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

Manage coordinates as tuples #5061

Merged
merged 2 commits into from
Oct 10, 2021
Merged

Conversation

michaelosthege
Copy link
Member

Model coordinates should be immutable objects, so tuples are prime candidates for that.

Unfortunately tuples cause problems when passed to xarray (#5043) which happens after the compute-intensive MCMC.

This PR modifies the Model.coords such that all coordinate values become tuples.
This way they're immutable and one can do model.coords["city"].index("London") to obtain indices that can be used for slicing tensors.

In the InferenceData conversion tuples are then automatically converted to NumPy arrays to avoid the problems with xarray.

Note that while this PR restricts the Model.coords to tuples, the user may still manually pass other types such as MultiIndex for the conversion.

Depending on what your PR does, here are a few things you might want to address in the description:

  • what are the (breaking) changes that this PR makes? → above
  • important background, or details about the implementation → above
  • are the changes—especially new features—covered by tests and docstrings? → yes
  • linting/style checks have been run → yes

@michaelosthege michaelosthege added bug trace-backend Traces and ArviZ stuff labels Oct 8, 2021
@michaelosthege michaelosthege added this to the v4.0.0-beta1 (vNext) milestone Oct 8, 2021
@michaelosthege michaelosthege self-assigned this Oct 8, 2021
@codecov
Copy link

codecov bot commented Oct 8, 2021

Codecov Report

Merging #5061 (4034162) into main (595e164) will decrease coverage by 11.97%.
The diff coverage is 94.44%.

❗ Current head 4034162 differs from pull request most recent head c9c07bb. Consider uploading reports for the commit c9c07bb to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##             main    #5061       +/-   ##
===========================================
- Coverage   78.43%   66.45%   -11.98%     
===========================================
  Files         130      130               
  Lines       24464    24441       -23     
===========================================
- Hits        19188    16243     -2945     
- Misses       5276     8198     +2922     
Impacted Files Coverage Δ
pymc/model.py 82.75% <75.00%> (-1.35%) ⬇️
pymc/backends/arviz.py 89.35% <100.00%> (-0.09%) ⬇️
pymc/tests/test_idata_conversion.py 96.31% <100.00%> (+0.01%) ⬆️
pymc/tests/test_bart.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/test_math.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/test_moment.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/test_shared.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/test_logprob.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/test_missing.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/test_tracetab.py 0.00% <0.00%> (-100.00%) ⬇️
... and 35 more

And always make them numpy arrays for the InferenceData conversion.

Closes pymc-devs#5043
@michaelosthege michaelosthege changed the title Enforce coordinates to be tuples Manage coordinates as tuples Oct 10, 2021
@michaelosthege michaelosthege merged commit 8796272 into pymc-devs:main Oct 10, 2021
@michaelosthege michaelosthege deleted the fix-5043 branch October 10, 2021 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug trace-backend Traces and ArviZ stuff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants