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

Enforce dims elements to be strings #6470

Merged
merged 1 commit into from
Jan 25, 2023

Conversation

ricardoV94
Copy link
Member

I think there was a confusion introduced in 64e63d8

Unless I am missing something, it's not possible to have arbitrary missing dims. Only works if missing dims are at the rightmost positions.

Closes #6379

@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Merging #6470 (41944f7) into main (00d2675) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #6470   +/-   ##
=======================================
  Coverage   94.79%   94.79%           
=======================================
  Files         148      148           
  Lines       27730    27732    +2     
=======================================
+ Hits        26287    26289    +2     
  Misses       1443     1443           
Impacted Files Coverage Δ
pymc/backends/arviz.py 96.37% <100.00%> (ø)
pymc/model.py 89.86% <100.00%> (+0.02%) ⬆️

@michaelosthege
Copy link
Member

I think #6379 was accurate: The problem was only in the InferenceData conversion and the solution Oriol suggested here should fix it.
The test you wrote should be able to confirm that

IIRC there are some valid use cases for None dims with multivariate RVs such as a MultivariateNormal, that's why we (and ArviZ) had this in the first place

@ricardoV94
Copy link
Member Author

There are other issues with None. For instance, model.add_coord assumes all dims are a string, but we are calling it with None dname from model.register_rv. It seems like the infrastructure to allow unspecified dims was not really worked out completely.

@michaelosthege michaelosthege added the major Include in major changes release notes section label Jan 23, 2023
@michaelosthege michaelosthege changed the title Enforce dims to be strings Enforce dims elements to be strings Jan 23, 2023
@OriolAbril
Copy link
Member

looks good.

In case it helps, here is some extra context on arviz and xarray. Neither of them support None as dimensions, but they support in some cases not providing dimensions or providing only part of them (in which case, the provided dims will be used to name the first n axis, the n+1 and more axis get default dim names). Moreover, xarray allows any hashable to be used as dimension name, not necessarly strings. ArviZ aims to not break when dims aren't strings (using reprs and str fallbacks if necessary) but it kind of assumes that dimension names are strings.

@ricardoV94 ricardoV94 merged commit 902b1ec into pymc-devs:main Jan 25, 2023
@ricardoV94 ricardoV94 deleted the check_dims_type branch June 6, 2023 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug major Include in major changes release notes section
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in conversion of None dims to InferenceData
3 participants