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

Assert ndim and number of dims match #7390

Merged
merged 2 commits into from
Jun 26, 2024

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Jun 25, 2024

Fixes bug where invalid number of dims was given to Deterministic introduced by OrderedLogistic/Probit

Description

Related Issue

  • Closes #
  • Related to #

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

📚 Documentation preview 📚: https://pymc--7390.org.readthedocs.build/en/7390/

@@ -1263,7 +1256,7 @@ class OrderedLogistic:
def __new__(cls, name, eta, cutpoints, compute_p=True, **kwargs):
p = cls.compute_p(eta, cutpoints)
if compute_p:
p = pm.Deterministic(f"{name}_probs", p, dims=kwargs.get("dims"))
p = pm.Deterministic(f"{name}_probs", p)
Copy link
Member Author

@ricardoV94 ricardoV94 Jun 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not trivial. p may have more or less dims than the respective Categorical variable, and these may or not be broadcasting. I decided it was not worth the trouble for the time being. In general we still need to figure out an approach for automatic dims (see #6828)

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 partial dim assignment is currently supported, so you could do something like dims=(None,) * dim_diff + ('categories', ), no? The last one at least is always known iiuc

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No this is not sufficient either because p could have a broadcastable dim on the same batch dimensions of the underlying categorical variable. pm.Categorical(..., p=[[0.5, 0.5]], dims="obs”). You cannot say p has dims ("obs", None) because it has shape (1, 2) not (2, 2)

Copy link
Member Author

@ricardoV94 ricardoV94 Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or you're suggestion adding a "categories dim" regardless of the rest. That has another problem, what if the user creates 2 variables with different number of categories? Then again you have two dims that are incompatible (same name, different shape).

That can also be solved (by using the variable name) but I don't want to do it here, I just want to not break stuff for now.

Besides that was not what the code was doing, it seems like the biggest focus was on reusing the user provided dims, not auto naming the extra dim

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was assuming that "categories" already was provided somewhere, not that we generate or auto-name anything.

But yes I understand this is a rabbit hole, focusing on not breaking things is fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case the PR is ready for review :)

@ricardoV94 ricardoV94 force-pushed the fix_ndim_ordered branch 2 times, most recently from 05258f4 to 818e814 Compare June 25, 2024 17:46
Copy link

codecov bot commented Jun 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.19%. Comparing base (b496127) to head (81f25c1).
Report is 83 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #7390   +/-   ##
=======================================
  Coverage   92.19%   92.19%           
=======================================
  Files         103      103           
  Lines       17214    17215    +1     
=======================================
+ Hits        15870    15871    +1     
  Misses       1344     1344           
Files with missing lines Coverage Δ
pymc/distributions/discrete.py 99.40% <100.00%> (-0.01%) ⬇️
pymc/model/core.py 91.77% <100.00%> (+0.02%) ⬆️

@ricardoV94 ricardoV94 merged commit d5ff424 into pymc-devs:main Jun 26, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants