search

static-frame / arraykit

Python C Extensions for StaticFrame
Other
8 stars 2 forks source link

Add get_new_indexers_and_screen function #63

Closed chaburkland closed 2 years ago

chaburkland commented 2 years ago

Implement get_new_indexers_and_screen. For use by static_frame.IndexHierarchy._extract_iloc.

chaburkland commented 2 years ago

@brandtbucher

I'm getting segfaults and I'm not sure why. Would you mind giving this a preliminary review to help me discover where I went wrong?

chaburkland commented 2 years ago

@brandtbucher

I'm getting segfaults and I'm not sure why. Would you mind giving this a preliminary review to help me discover where I went wrong?

I figured out what my issue was, so there are no more segfaults! However, I would still appreciate a review if you have time :)

flexatone commented 2 years ago

@chaburkland : what was the issue that caused the segfaults?

chaburkland commented 2 years ago

@chaburkland : what was the issue that caused the segfaults?

While I'm not 100% sure, I believe it's because my implementation has a strong assumption that the incoming array is int64, as we do a C-cast on the underlying data buffer. This check was not validated, and so for certain architectures where the default integers are not int64, I think the cast was leading to instances where invalid memory was being accessed.

I'm not sure exactly where the failure was happening, but it appears all the segfaults happened on 32bit architectures (or Windows), and after I updated all my examples to use int64, and added an explicit type check at the beginning of the function, everything ran successfully.

chaburkland commented 2 years ago 
<class '__main__.GetNewIndexersAndScreenPerf'>
cls                             func             ak              ref              ref/ak     
GetNewIndexersAndScreenPerf     ordered          0.56844831      5.97792536       10.51621616
GetNewIndexersAndScreenPerf     unordered        0.42423423      10.39049229      24.49234776
GetNewIndexersAndScreenPerf     tiled            0.38999344      8.84025321       22.6676972 
GetNewIndexersAndScreenPerf     repeat           0.42471759      7.42528738       17.48288185
GetNewIndexersAndScreenPerf     quick_exit       0.07150431      4.37841167       61.23283912
GetNewIndexersAndScreenPerf     late_exit        0.73353364      11.87805548      16.19292536
GetNewIndexersAndScreenPerf     small            0.53452164      10.94190227      20.470457  
GetNewIndexersAndScreenPerf     large            0.27184065      5.28212683       19.43096713
chaburkland commented 2 years ago 
<class '__main__.GetNewIndexersAndScreenPerf'>
cls                             func             ak              ref              ref/ak
GetNewIndexersAndScreenPerf     ordered          65.87289547     1156.38919865    17.55485607
GetNewIndexersAndScreenPerf     unordered        41.42570534     2004.67702913    48.39210371
GetNewIndexersAndScreenPerf     tiled            33.83376436     1739.00276589    51.39844173
GetNewIndexersAndScreenPerf     repeat           36.48492943     1423.43519807    39.01433333
GetNewIndexersAndScreenPerf     quick_exit       5.54228165      855.40508353     154.341684
GetNewIndexersAndScreenPerf     late_exit        64.83247354     2305.70365384    35.56402414
GetNewIndexersAndScreenPerf     small            37.22550323     2132.38490796    57.28290347
GetNewIndexersAndScreenPerf     large            33.07465854     1029.98867426    31.1413245
flexatone commented 2 years ago

@chaburkland : if you think this is ready lets merge it and get a new release out.