-
Notifications
You must be signed in to change notification settings - Fork 27
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
Fix for scrambled order of columns #73
Conversation
Hi, @AStupidBear a fix here I think. I copied latter commit from another package, it was slightly different with: DataStructures = "0.17, 0.18" |
Just merge this? I'm pretty sure this works. My "but is there a better way?" RFC was about efficiency, while not too important here. |
I think it's better to wait for @malmaud (although technically I can hit the merge button). Until then, I think adding a test would help to smooth the review. Maybe test that the order of columns is preserved with the data frames with columns (say) Also, since Pandas.jl needs PyCall.jl anyway, I guess we can use PyDict to avoid relying on OrderedCollections.jl here just for one line? Alternatively, it also should be possible to add a column to |
If I get my way and Dict will be OrderedDict in Base, then this wouldn't be needed, as of next Julia LTS. But the dependency is light, and may even end up as a stdlib, so I wouldn't worry about that part. About a test: OrderedDict means the order couldn't be different. |
Thanks! I agree with @tkf - this needs a test and I'd rather avoid adding a dependency. |
I did try to avoid this cheap dependency. I probably spent way more time on that (and also the rest) than I have time for now, but couldn't see a good way. It's at least very simple this way, and I'm already using it by using my own fork, since not merged here. My priority now, is well my job, and I already made the PR to JuliaLang to make OrderedDict the default, but it may need followup/convincing people, as I want it in Julia 1.6 but if wouldn't help here for users on older versions. |
9a071e9
to
18ebf3f
Compare
Co-authored-by: Páll Haraldsson <Pall.Haraldsson@gmail.com>
Codecov Report
@@ Coverage Diff @@
## main #73 +/- ##
=======================================
Coverage ? 66.00%
=======================================
Files ? 5
Lines ? 253
Branches ? 0
=======================================
Hits ? 167
Misses ? 86
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
Thanks Pall, I just added a test and will merge this now. |
Fixes: #72