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

[FS-1042] Implement transpose on Seq, List and Array #4020

Merged
merged 5 commits into from
Dec 18, 2017

Conversation

PatrickMcDonald
Copy link
Contributor

@PatrickMcDonald PatrickMcDonald commented Nov 26, 2017

@abelbraaksma
Copy link
Contributor

I haven't checked the code, but I think this is a valuable addition to the library!

try
yield seq { while ie.MoveNext() do
let inner = ie.Current.GetEnumerator()
if inner.MoveNext() then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the first inner.MoveNext is false then inner never gets added to rm and thus never gets disposed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel something is odd about this whole implementation. If I do a Seq.toArray on the overall result, without iterating each individual sequence, then none of the inner enumerators have been created, and rm will not have been populated. But we will have already executed the outer finally` designed to cleanup the rm`` array. This feels wrong. I suspect you may just have to create a strict data structure and return simpler iterators.

Copy link
Contributor Author

@PatrickMcDonald PatrickMcDonald Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good points, this implementation was kind of a brain dump, while trying not to eagerly evaluate any sequences. I'll take a look at it later on and try and fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I've been playing around with Seq.transpose, something like the following works, but not for infinite lists

    [<CompiledName("Transpose")>]
    let transpose (source: seq<seq<'T>>) =
        checkNonNull "source" source
        source
        |> collect indexed
        |> groupBy fst
        |> map (snd >> (map snd))

If we were to allow one dimension of the source to be infinite, which would it be?
n x inf or inf x n

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PatrickMcDonald The above is very natural, I would go with that, at least on first look

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be possible to make it work with both infinite dimensions, however it will require maintaining a buffer of enumerators, but I would be an interesting feature.

@PatrickMcDonald PatrickMcDonald changed the title Implement transpose on Seq, List and Array WIP: Implement transpose on Seq, List and Array Nov 28, 2017
@dsyme dsyme changed the title WIP: Implement transpose on Seq, List and Array [ WIP, RFC FS-1042 ] Implement transpose on Seq, List and Array Dec 1, 2017
@dsyme dsyme changed the title [ WIP, RFC FS-1042 ] Implement transpose on Seq, List and Array [ WIP, FS-1042] Implement transpose on Seq, List and Array Dec 1, 2017
@dsyme dsyme changed the title [ WIP, FS-1042] Implement transpose on Seq, List and Array [WIP, FS-1042] Implement transpose on Seq, List and Array Dec 1, 2017
@KevinRansom KevinRansom changed the base branch from master to dev15.6 December 7, 2017 18:57
@PatrickMcDonald PatrickMcDonald changed the title [WIP, FS-1042] Implement transpose on Seq, List and Array [FS-1042] Implement transpose on Seq, List and Array Dec 7, 2017
Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this.

@ dsyme ... you good with this. For 4.3?

@KevinRansom KevinRansom merged commit a2c1bc0 into dotnet:dev15.6 Dec 18, 2017
@PatrickMcDonald PatrickMcDonald deleted the transpose branch December 18, 2017 18:15
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