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

Simplified __call__ interface? #64

Closed
bmcfee opened this issue Mar 18, 2017 · 12 comments
Closed

Simplified __call__ interface? #64

bmcfee opened this issue Mar 18, 2017 · 12 comments

Comments

@bmcfee
Copy link
Collaborator

bmcfee commented Mar 18, 2017

I've been using pescador a bit over the last few weeks, and find myself wondering if we could tighten up the API a little before the 1.0 release.

Specifically, streamer.generate() is a bit verbose; how about adding a __call__ method so we can instead do something like:

stream = pescador.Streamer(my_generator, my_params...)

for x in stream():
    process(x)

This wouldn't help for cycle or tuples, but it would cut down a bit of boilerplate code for the most common use cases.

@bmcfee bmcfee added this to the 1.0 milestone Mar 18, 2017
@cjacoby
Copy link
Collaborator

cjacoby commented Mar 18, 2017

I've also been using it a lot recently, and I like. 👍

What if we kept the generator() interface internally, and did something like this?

def __call__(self, cycle=False, **kwargs):
    if cycle:
        return self.cycle(**kwargs)
    else:
        return self.generate(**kwargs)

@bmcfee
Copy link
Collaborator Author

bmcfee commented Mar 18, 2017

I dig it. Tuples are still an oddball, and I'd prefer to keep that one separate.

How about I implement this after merging the doc updates so that everything can be updated in one pass?

@cjacoby
Copy link
Collaborator

cjacoby commented Mar 18, 2017 via email

@bmcfee bmcfee closed this as completed in ca4dca7 Mar 18, 2017
@bmcfee bmcfee reopened this Mar 18, 2017
@ejhumphrey
Copy link
Collaborator

I realize I slept on this issue the first time around and apologize in advance, but I would be keen to revisit this discussion maybe? 😞 🙏

Specifically, I'd propose making core.Streamer directly iterable, e.g. [x for x in stream] is a thing.

Commentary

  • This isn't (necessarily) mutually exclusive with [x for x in stream(max_batches=10, cycle=True)] ... though it does violate the "There should be one good way to do a thing" principle.
  • My most common use case is consistent with the default args (which leads me to think that it is the common use case) ... which means everyone using pescador will be writing for stuff in stream() all over the place, and those bonus parens smell like a minor anti-pattern.
  • Can the cycle and max_batches (or max_samples, re: Language conventions #75) go in the init method? I don't immediately see a reason these would change over the lifetime of a Streamer, but am happy to be educated.

@ejhumphrey ejhumphrey reopened this Apr 3, 2017
@bmcfee
Copy link
Collaborator Author

bmcfee commented Apr 3, 2017

Can the cycle and max_batches (or max_samples, re: #75) go in the init method? I don't immediately see a reason these would change over the lifetime of a Streamer, but am happy to be educated.

I think this might cause some difficulties with how streamers get used inside Mux. If you're using poisson-distributed sample limits, you would have a different max_batches each time the streamer is activated. Ditto for cycle, though it's a bit less obvious there.

Given the above, I'm not sure dropping the call interface (or implementing a direct iterable interface) is the best option, but I'm open to persuasion.

@ejhumphrey
Copy link
Collaborator

well, here's some simple(r) math: what's the more annoying anti-pattern, a high occurrence of empty parens, or two call signatures for the same functionality?

@cjacoby
Copy link
Collaborator

cjacoby commented Apr 3, 2017 via email

@ejhumphrey
Copy link
Collaborator

hrm, well in that case, I would augment my proposal to:

  • Iterating on a Streamer behaves as one would expect (it raises StopIteration and runs to exhaustion), and one of...
  • (a) Explicitly calling the streamer (__call__) with args allows the Streamer to augment it's behavior, such as cycle itself or terminate early via max_iter, which I would assume violates PEP conventions around iterators or (b) do we need __call__ when we have both generate() and cycle()?

@cjacoby
Copy link
Collaborator

cjacoby commented Apr 3, 2017 via email

@ejhumphrey
Copy link
Collaborator

having thought about it more, I agree with you on 👍 (a) ... a simplified / concise interface for doing non-standard things is nice, and I can imagine that it'd be useful to control these parameters with config-args, rather than augmenting code.

@ejhumphrey
Copy link
Collaborator

tiny update: I'm having a really interesting / useful day catching up to pescador. Given the mix of Streamer and generator objects, I would like to emphasize the need for anything that gets referred to as a "stream" to be directly iterable.

@bmcfee bmcfee removed this from the 1.0 milestone Apr 9, 2017
@ejhumphrey
Copy link
Collaborator

closed with #88

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants