-
Notifications
You must be signed in to change notification settings - Fork 6
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 Traversing instance for Cell #95
Conversation
Before the last commit my version of traverse' was not as strict as
After the last commit both of the expressions above result in bottom. traverse' for Cell uses strict StateT which seems to be as strict as resampleList. On the other hand traverse' for ArrM is not as strict as resampleList, as traverse is not strict. To correct this a strict version of traverse is used. The code from that is taken from code from this post. |
62e12df
to
b3f78af
Compare
Refactored to its own module |
I'm unsure about the strictness. I agree that there should be some strictness, but I think the examples shouldn't be so strict. In fact, I'm more and more convinced that I implemented Some code examples: testSetup x = take 4 $ head $ fst $ runIdentity $ steps (traverse' (arr id) :: Cell Identity [Int] [Int]) x
shouldNotBeBottom = testSetup [[1..],[2],[3]]
shouldBeBottom = testSetup [1 : error "boom", [2], [3]] I think it should only diverge if there is a a problem in the list, but not if there is a problem in the data. That way we can be sure the list is processed completely, but leave it to the cell itself to decide whether it needs to evaluate all the data inside. Can you add a few tests that capture the expected behaviour? If you use |
I had implemented it this way to match the strictness in
Ok, in that case I will change to using |
Will that evaluate the list structure strictly? I'd expect it doesn't. |
Some background: I introduced the additional strictness in #19. In fact, we have strictness in the data passing through cells: Cell state2 step2 . Cell state1 step1 = Cell { .. }
where
cellState = Composition state1 state2
cellStep (Composition state1 state2) a = do
(!b, state1') <- step1 state1 a
(!c, state2') <- step2 state2 b
return (c, Composition state1' state2') This surprises me a lot. I think this should be rethought first. |
The traversal of the traversable structure is lazy when using State.Lazy: Prelude Control.Monad.Trans.State.Lazy> take 2 $ fst $ flip runState 0 $ traverse (\_ -> modify (+2) >> get) [1..]
[2,4] On the other hand from the code of the composition of Cell above it is probably going to evaluate the whole list before passing it on to the next cell. So the behavior might be different if we just test a single cell or if we test the composition of two cells (i.e. But I suspect it won't matter much if we use Strict or Lazy State when the cell is composition of two cells... |
I've refactored the PR:
This implementation keeps the list created in Check if this addresses all the issues. |
d6ec722
to
e32ce9d
Compare
I'd prefer it if
So an input of Can you check whether the |
Sure, no problem, will do that :). If the user needs to use profunctor's Choice then they must import qualified by themselves, and profunctor's Choice probably is not going to be used.
Yes, it doesn't crash. The tests that I added are testing this. , testCase "traverse' by itself does not force the entire list (Cell)" $
testError "Traverse' is too strict" id $
head $ head $ fst $ runIdentity $ steps (traverse' cellId :: Cell Identity [Int] [Int]) [[1, error "Bang !"]]
I profiled SpeedTest and I didn't see any space leak. The output has a NaN, is that normal ? Looking at the code of SpeedTest, I think that it doesn't use any resample function, so there's no reason why it should change behavior right ?
|
97ff851
to
c2ba8c2
Compare
I think that's not the same thing what I meant. The test I meant would have as input not
Awesome, thanks :)
Yes, I think at some point it exceeds the limits of a
True, I'm puzzled a bit. I thought it used it somewhere, but maybe that was an older temporary version of the test. Or maybe I simply changed the definition of Anyways. Your choice of lazyness seems to be a good one then :) let's keep it like it is now. |
f35001f
to
3345bc8
Compare
I've added a mechanism to succinctly generate tests for all Traversable instances of interest. It uses induction via type-classes to go through a type-level list and generate a test for each type. If there are any other Traversables that we are interested in, I can add them to the list. Also, I rebased on top of master. issues:
related to
|
3345bc8
to
35b5ea2
Compare
Since more PRs area appearing, it would be nice to be able to finish this one :-). Our last discussion was about the tests. |
Yes :) I'm waiting for your changes. |
Ah, sorry, I think I missed something :-). which changes are you referring to ? |
If you scroll up a bit, you should see a lot of pending discussions 😉 Mostly of stylistic nature. |
Hi @turion :-). I went through all the discussions above again, there were one or two which were still marked unresolved, and I changed them now to resolved, but in any case I believe my last push from March had hopefully already addressed all the issues (at least that was my intention :-) ). In any case, I might have missed something, off course. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh, my bad! I didn't click on "Submit review"! So only i saw my missing comments. Sorry for delaying this for so long.
Add instances for Cell of Profunctor, Choice and Strong via WrappedArrow Add instance for Cell of Traversing
…raversable of interest.
35b5ea2
to
b46a6be
Compare
Sorry for leaving this PR abandoned for so long :-). I've tried to address all the points of the previous review. There are still 2 unresolved conversations above, but I've pushed my current version of the code. |
Ok, I think I have addressed all the issues of the last review. |
Thanks for the review ! Will do the changes and report back. |
Hopefully I have addressed the suggestions in the last review. As always, note that when you think this is ready to merge I can first squash que commits, but for now I'm leaving the individual commits to make it easier to see my latest changes. |
Hi, perhaps this is ready to merge ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your work and your patience!
Great ! It took a while, but is was worth discussing things and polishing the code ! Thanks for your reviews ! :-) |
Hopefully this solves #56 and #57
With this instance should
resampleList
andresampleMaybe
be removed ?