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

Do we need a VisitiationFrequency? #105

Closed
Datseris opened this issue Sep 27, 2022 · 2 comments
Closed

Do we need a VisitiationFrequency? #105

Datseris opened this issue Sep 27, 2022 · 2 comments
Labels
discussion-design Discussion or design matters

Comments

@Datseris
Copy link
Member

I'm a bit unhappy that we need to do the complicated VisitationFrequency(RectangularBinning(n)). I understand that VisitationFrequency could take other things than RectangularBinning, but I don't understand where else RectangularBinning can be given to except VisitationFrequency. Why don't we allow RectangularBinning itself to be used directly with probabilities? @kahaaga

@Datseris Datseris added the discussion-design Discussion or design matters label Sep 27, 2022
@kahaaga
Copy link
Member

kahaaga commented Sep 27, 2022

RectangularBinning is used both with VisitationFrequency and TransferOperator - that's why I didn't make RectangularBinning itself an estimator.

There will be two types of binnings: RectangularBinning and TriangularBinning (#55, which I have to finish after a loong hiatus). Both can be inputs to both VisitationFrequency and TransferOperator.

I do agree, however, that it would be convenient to just pass RectangularBinning directly to probabilities. We could just mention in the docstring that this is just a convenience call to VisitationFrequency.

On the same note, I think I'd like shorter names. What do you think about RectangularBinning → Grid and TriangulationBinning → Triangulation?

@kahaaga
Copy link
Member

kahaaga commented Oct 29, 2022

Why don't we allow RectangularBinning itself to be used directly with probabilities? @kahaaga

I guess we can close this, since we agreed in #127 that probabilities should only accept a ProbabilityEstimator?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion-design Discussion or design matters
Projects
None yet
Development

No branches or pull requests

2 participants