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

Multitraces should iterate over samples from all chains #2460

Closed
wants to merge 3 commits into from

Conversation

AustinRochford
Copy link
Member

Resolves #2457

@ferrine
Copy link
Member

ferrine commented Jul 31, 2017

Does it affect this code somehow?

@junpenglao
Copy link
Member

Does it affect this code somehow?

I think it would.

@AustinRochford
Copy link
Member Author

AustinRochford commented Jul 31, 2017

@ferrine it's not obvious to me that it will affect that code, but the tests will let us know for sure. I certainly expect some to fail, and will also be adding a few new ones before merging.

@ferrine
Copy link
Member

ferrine commented Jul 31, 2017

@AustinRochford
Copy link
Member Author

Thanks for the heads up, that one's a simple enough change, at least. I'll wait to see how many other places there are that depend on the old len. If there are many, I may need to take a different approach.

@AustinRochford
Copy link
Member Author

Coverage decreased by ~40%

image

@aseyboldt
Copy link
Member

Not sure what we should do here, all options I can see are a bit strange.

  • We could keep the current behaviour (len and iter ignore all but the first chain), but add a method that returns an iterator over all samples
  • Keep len as it is, but change iter to return all samples (the current PR). This kind of breaks common assumptions, usually len(list(obj)) should match len(obj) for all iterables.
  • Change both len and iter to return all samples

I think the first one might be a good compromise between reasonable behaviour and backward compatibility.

@AustinRochford
Copy link
Member Author

@aseyboldt I agree. I mostly want to see which tests this approach breaks. After trying the third option, which seems to break many parts of the code base, I'm inclined to go with the first, although it is a bit unsatisfying.

@twiecki
Copy link
Member

twiecki commented Aug 1, 2017

Hm, yeah, first option seems the most realistic.

@ferrine
Copy link
Member

ferrine commented Aug 1, 2017

I vote for the third

@aseyboldt
Copy link
Member

Any updates on this? We still haven't decided which way we'd like to go, right?

@AustinRochford
Copy link
Member Author

@aseyboldt this got lost in my vacation :|. I think we should go with the first option. Will try to get a new PR up in the next few days.

@junpenglao
Copy link
Member

@AustinRochford any progress? Would be great to fix this before 3.2 ;-)

@AustinRochford
Copy link
Member Author

Yes it would. Unfortunately no progress yet, but I'll see about putting something together tonight.

@junpenglao junpenglao deleted the multitrace-iter-all-chains branch September 15, 2017 08:04
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.

5 participants