Skip to content

MarkovCombinator - a combinator with a type-enforced restricted dependency pattern (for parallel density evaluation) #1426

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

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

femtomc
Copy link
Collaborator

@femtomc femtomc commented Nov 4, 2024

No description provided.

@femtomc femtomc requested a review from MathieuHuot November 4, 2024 17:45
@MathieuHuot
Copy link
Collaborator

@femtomc as we discussed we need to rename it HMM combinator as StateSpace is the general case and HMM is the one with the restrictions you are assuming for this combinator.

@femtomc
Copy link
Collaborator Author

femtomc commented Nov 5, 2024

@MathieuHuot How about MarkovCombinator?

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 48.97959% with 50 lines in your changes missing coverage. Please review.

Project coverage is 87.69%. Comparing base (8d8b677) to head (0aaa00a).

Files with missing lines Patch % Lines
...ax/_src/generative_functions/combinators/markov.py 48.45% 50 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1426      +/-   ##
==========================================
- Coverage   88.64%   87.69%   -0.96%     
==========================================
  Files          55       56       +1     
  Lines        3980     4078      +98     
==========================================
+ Hits         3528     3576      +48     
- Misses        452      502      +50     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MathieuHuot
Copy link
Collaborator

MathieuHuot commented Nov 5, 2024

Would that be possible to add an optional static boolean arg to the scan combinator called "markov" instead of the whole new combinator? Then we'd branch on this value for the bits of code that are specific to this combinator, which I think is mainly assess here.

@femtomc
Copy link
Collaborator Author

femtomc commented Nov 5, 2024

Two things:

  • This PR isn't done yet, I need to add tests, and fill in the full GFI
  • We should keep specialized logic separate -- as that is the entire point of Gen -- little modular pieces.

Now, on point (2) -- we could think of some way for someone to always be able to use scan as a shorthand, but sometimes get a MarkovCombinator back. That's fair game -- and a useful discussion (how to reduce how much users need to know to get optimized combinators into their code).

@femtomc femtomc marked this pull request as draft November 5, 2024 04:32
@femtomc
Copy link
Collaborator Author

femtomc commented Nov 5, 2024

Changing to draft to reflect the fact that it's not done yet, will switch it back when I finish it tomorrow.

@MathieuHuot
Copy link
Collaborator

Two things:

  • This PR isn't done yet, I need to add tests, and fill in the full GFI
  • We should keep specialized logic separate -- as that is the entire point of Gen -- little modular pieces.

Now, on point (2) -- we could think of some way for someone to always be able to use scan as a shorthand, but sometimes get a MarkovCombinator back. That's fair game -- and a useful discussion (how to reduce how much users need to know to get optimized combinators into their code).

I'm not convinced by (2). What I'm saying should be strictly equivalent in terms of capabilities and actual new code content, it should mostly avoid a lot of code duplication and unnecessary boiler plate.

@MathieuHuot
Copy link
Collaborator

Two things:

  • This PR isn't done yet, I need to add tests, and fill in the full GFI
  • We should keep specialized logic separate -- as that is the entire point of Gen -- little modular pieces.

Now, on point (2) -- we could think of some way for someone to always be able to use scan as a shorthand, but sometimes get a MarkovCombinator back. That's fair game -- and a useful discussion (how to reduce how much users need to know to get optimized combinators into their code).

I'm not convinced by (2). What I'm saying should be strictly equivalent in terms of capabilities and actual new code content, it should mostly avoid a lot of code duplication and unnecessary boiler plate.

I see another advantage. instead of having a lot of "non-implemented error" for update/generate etc, all the ones from scan should still be valid. This combinator is really a code specialization one, leveraging the structure of a model. So we now leverage it for assess. Maybe we could then leverage this more for some EditRequests that could check whether the think is Markov, and either only be valid if the thing is Markov (like IndexRequest perhaps?), or specialize the EditRequest to do something more efficient (assess could technically be an EditRequest so that one could already fall into that category).
Finally, say later we find another specialized implementation of generate that leverages the fact the model is Markov, then it'd again be an easy addition to the codebase.

@femtomc femtomc changed the title StateSpaceCombinator - a combinator with a type-enforced restricted dependency pattern (for parallel density evaluation) MarkovCombinator - a combinator with a type-enforced restricted dependency pattern (for parallel density evaluation) Nov 7, 2024
@femtomc
Copy link
Collaborator Author

femtomc commented Nov 7, 2024

@MathieuHuot I see -- I misunderstood your proposal initially, but I think I understand it.

Maybe what you're saying is that Markov should be a specialization of Scan (so e.g. it could wrap Scan -- and re-use its implementation for various methods). I need to think about how this could work, but I agree with you on its benefits.

To make it work, we have to solve problems related to what the signature of the scan kernel expects, and the signature of this Markov combinator kernel expects (they are different).

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