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

Add logprob inference for not operations #6689

Merged

Conversation

shreyas3156
Copy link
Contributor

@shreyas3156 shreyas3156 commented Apr 26, 2023

What is this PR about?
Implementation of logprob derivation for bitwise 'not' operations as discussed in #6680, using the following logic:
not(gt(x, const), True) -> (gt(x, const), not(True)).

Checklist

New features

  • Logprob inference for bitwise 'not' Ops.

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

@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

Merging #6689 (fd6ddde) into main (55d915c) will decrease coverage by 1.46%.
The diff coverage is 96.15%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6689      +/-   ##
==========================================
- Coverage   91.99%   90.54%   -1.46%     
==========================================
  Files          95       95              
  Lines       16016    16075      +59     
==========================================
- Hits        14734    14555     -179     
- Misses       1282     1520     +238     
Impacted Files Coverage Δ
pymc/logprob/binary.py 96.66% <96.15%> (-0.31%) ⬇️

... and 6 files with indirect coverage changes

Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

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

Looks good, some small tweak suggestion below

pymc/logprob/binary.py Outdated Show resolved Hide resolved
pymc/logprob/binary.py Show resolved Hide resolved
pymc/logprob/binary.py Outdated Show resolved Hide resolved
@shreyas3156 shreyas3156 force-pushed the not-operation-logprob-inference branch from 1e76e60 to 7222241 Compare April 27, 2023 10:05
return None

if not base_var.dtype.startswith("bool"):
raise TypeError("Base RV should be of type boolean")
Copy link
Member

@ricardoV94 ricardoV94 Apr 27, 2023

Choose a reason for hiding this comment

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

Suggested change
raise TypeError("Base RV should be of type boolean")
raise TypeError("Base RV should be of type boolean")

We shouldn't raise errors, because another rewrite (including a user custom one) could implement a transformation for this type of variable.

One thing we might want to start doing sooner or later is to add log.debug statements so that users can figure out why didn't pymc figure out the logprob for their expression (where here we could say that bitwise for integer variables is not supported by this rewrite).

Copy link
Member

Choose a reason for hiding this comment

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

Opened an issue #6691

Here we should just return None, which means the current rewrite does not apply

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We shouldn't raise errors, because another rewrite (including a user custom one) could implement a transformation for this type of variable.

Understood. I'll replace it to return None.

One thing we might want to start doing sooner or later is to add log.debug statements so that users can figure out why didn't pymc figure out the logprob for their expression (where here we could say that bitwise for integer variables is not supported by this rewrite).

I have a question about this. Since the logprob rewrites are in the IR graph, should the user be exposed to the internal errors? (e.g.: If the value variable is potentially measurable).

Copy link
Member

Choose a reason for hiding this comment

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

Since the logprob rewrites are in the IR graph, should the user be exposed to the internal errors? (e.g.: If the value variable is potentially measurable).

What do you mean by internal errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking of a scenario when we check the potential measurability of the value variables and whether the user would be concerned with the error. But I figured that it has to be the expression defined by the user that would cause the error, so the answer should be yes. It may have been an oversight on my part since I'm still getting acquainted with the use cases of PyMC.

Copy link
Member

@ricardoV94 ricardoV94 Apr 28, 2023

Choose a reason for hiding this comment

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

The point I was trying to make is that these rewrites are local. So what looks like something invalid to them, may be valid for another rewrite (ours or provided by the user). So we shouldn't raise any errors ourselves, but simply return None, which states: this rewrite does not apply to this graph.

What I suggested in the linking issue, is that we can issue a log message saying why this local rewrite was not applied, as it can be very useful for the user to understand why the logprob was not derived for their graph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I understand now. Thank you so much for clarifying!

@shreyas3156 shreyas3156 force-pushed the not-operation-logprob-inference branch from 7222241 to fd6ddde Compare April 27, 2023 17:24
@ricardoV94 ricardoV94 merged commit 371472d into pymc-devs:main Apr 28, 2023
@ricardoV94 ricardoV94 changed the title Add logprob inference for not operations Add logprob inference for not operations May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants