search

fslaborg / Deedle

Easy to use .NET library for data and time series manipulation and for scientific programming
http://fslab.org/Deedle/
BSD 2-Clause "Simplified" License
929 stars 196 forks source link

Don't reverse columns and rows in the FrameBuilder #321

Closed filmor closed 8 years ago

filmor commented 8 years ago

It's very unintuitive that FrameBuilder reverses columns and rows before creating the actual frame. I'd expect it to be a thin wrapper around Frame.ofRows and Frame.ofColumns with the same semantics.

tpetricek commented 8 years ago

Hmm, does it actually reverse the columns and rows?

The implementation of Add is adding the new columns/rows at the beginning of the list, so that's why there is a call to List.rev. So my naïve reading is that this should not be reversing them... but there must be something weird happening here!

Could you add a test case to the C# tests?

filmor commented 8 years ago

This is really weird. You are absolutely right, this does work in master as expected. However, it does certainly not work here.

I'm using version 1.2.3 of Deedle here and the FSharp.Core.3 nuget package (just tried with FSharp.Core and the "system" FSharp, same result). Any hints?

tpetricek commented 8 years ago

Sorry, I was completely stupid!

The code includes List.rev (as it should) in GetEnumerator, but the reversing is missing in the Frame property (which is what you are testing and what was, indeed, not working correctly). I cherry-picked your commit with the test & fixed this in #322 so that I can include it in a release now. Thanks a lot for spotting this :-).