use fused types for parts of algos_common_helper #22452

jbrockmendel · 2018-08-21T22:02:35Z

Broken off of #22432, which was a proof of concept.

gfyoung

WillAyd · 2018-08-21T23:29:11Z

pandas/_libs/algos.pyx

+    return indexer
+
+
+pad_float64 = pad["float64_t"]


Do we need to explicitly use indexing here or is calling also an option? The latter may help reduce the amount of code required (maybe worth exploring in separate PR):

http://docs.cython.org/en/latest/src/userguide/fusedtypes.html#calling

Do we need to explicitly use indexing here or is calling also an option?

I think you're right that the calling can be simplified, but this way keeps the changes self-contained.

Yea for sure - not saying do here but food for thought in subsequent changes or new developments.

WillAyd · 2018-08-21T23:45:34Z

pandas/_libs/algos.pyx

+        return False, False, True
+
+    if algos_t is not object:
+        with nogil:


Since this is just copy / paste of the below block, what do you think about creating a blank context manager that doesn't really do anything.

if algos_t is not object: cm = nogil else: cm = dummy_context

And then either implementation can share the context manager:

with cm: ...

Would reduce the copy / paste and make it so we don't miss updates to say one part of the fused type in the future that makes for hard-to-find implementation differences. Could be generalizable in a few instances in Cython.

That would be great if it can be made to work; a lot of the tempita code takes this form.

I don't think cython treats with nogil as an actual contextmanager. @scoder any thoughts on this?

I agree, making this simpler would motivate this PR

Well, nogil is really special in Cython and not an actual context manager. I could imagine something like with nogil.only_if(some_bool_condition), which would then duplicate the with-block to generate a True and False version of it and select the right one at runtime. Or even at compile time, if the condition is a compile time constant, such as a type check on fused types (release GIL for native C types, keep for object types). The latter would definitely be possible, whereas duplicating the code block could be messy and is definitely more work.

Note that nogil is also a decorator, so nogil(something) is already taken.

@scoder thanks for the info! What would be the next steps to get the functionality you've described? Compile-time evaluation would certainly be ideal

Read and discuss Allow (compile-time) conditional "with nogil" blocks cython/cython#2579

find someone to implement it

:)

Looks like this will be released in Cython 3.0 - could have a few uses in our codebase for sure. Thanks!

codecov · 2018-08-22T01:18:18Z

Codecov Report

Merging #22452 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #22452   +/-   ##
=======================================
  Coverage   92.05%   92.05%           
=======================================
  Files         169      169           
  Lines       50733    50733           
=======================================
  Hits        46702    46702           
  Misses       4031     4031

Flag	Coverage Δ
#multiple	`90.46% <ø> (ø)`	⬆️
#single	`42.24% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68273a7...749989f. Read the comment docs.

jreback · 2018-08-22T10:12:10Z

pandas/_libs/algos.pyx

+        return False, False, True
+
+    if algos_t is not object:
+        with nogil:


I agree, making this simpler would motivate this PR

jreback · 2018-08-22T10:12:34Z

pandas/_libs/algos_common_helper.pxi.in

-    return maybe_convert_objects(result)
-
-{{endfor}}
-
 #----------------------------------------------------------------------


what's left here? possible to fully remove this file?

AFAICT diff_2d_dtpye is not straightforward to using fused types

jreback · 2018-08-22T10:12:47Z

perf check?

jbrockmendel · 2018-08-22T14:02:15Z

perf check?

Still need to run, will update later in the day.

jbrockmendel · 2018-08-22T14:04:07Z

I agree, making this simpler would motivate this PR

With this option it would be a slam dunk. Without it I still think this is preferable, but it's a closer call.

jbrockmendel · 2018-08-22T17:59:59Z

asvs look on the good size of noise:

taskset 6 python2 -m asv continuous -f 1.1 -E virtualenv master HEAD -b monotonic -b pad -b fill

    before     after       ratio
  [25e6a21a] [749989fa]
+   41.98ms    71.36ms      1.70  strings.Methods.time_pad
+  421.04ns   495.46ns      1.18  categoricals.IsMonotonic.time_categorical_index_is_monotonic_increasing
+   36.21μs    42.47μs      1.17  categoricals.IsMonotonic.time_categorical_series_is_monotonic_increasing
-   55.63μs    50.47μs      0.91  categoricals.IsMonotonic.time_categorical_series_is_monotonic_decreasing
-  454.94ns   342.00ns      0.75  categoricals.IsMonotonic.time_categorical_index_is_monotonic_decreasing

    before     after       ratio
  [25e6a21a] [749989fa]
+   39.64μs    48.52μs      1.22  categoricals.IsMonotonic.time_categorical_series_is_monotonic_increasing
+  307.82ns   369.50ns      1.20  categoricals.IsMonotonic.time_categorical_index_is_monotonic_decreasing
-    3.54ms     3.10ms      0.88  replace.FillNa.time_fillna(False)
-   53.65μs    44.85μs      0.84  categoricals.IsMonotonic.time_categorical_series_is_monotonic_decreasing
-   95.93ms    79.04ms      0.82  join_merge.Align.time_series_align_left_monotonic
-    1.96ms     1.49ms      0.76  replace.FillNa.time_fillna(True)
-  455.37ns   309.91ns      0.68  categoricals.IsMonotonic.time_categorical_index_is_monotonic_increasing
-   72.43ms    41.78ms      0.58  strings.Methods.time_pad

before     after       ratio
  [25e6a21a] [749989fa]
+    1.50ms     1.87ms      1.24  replace.FillNa.time_fillna(True)
-  520.84ns   463.43ns      0.89  categoricals.IsMonotonic.time_categorical_index_is_monotonic_decreasing
-    7.20ms     6.15ms      0.86  frame_methods.Fillna.time_frame_fillna(False, 'pad')
-   61.60μs    43.60μs      0.71  categoricals.IsMonotonic.time_categorical_series_is_monotonic_decreasing
-  474.23ns   323.60ns      0.68  categoricals.IsMonotonic.time_categorical_index_is_monotonic_increasing
-   65.39ms    39.47ms      0.60  strings.Methods.time_pad

jbrockmendel · 2018-09-07T16:24:10Z

Looks like rebasing is gonna be a hassle. Before I jump into it, is the not-yet-implemented cython feature a deal-breaker? I prefer the implementation in this PR, but I'm not clear on what the consensus is.

WillAyd · 2018-09-07T19:34:50Z

I don’t consider it a deal breaker because I’d seen that same copy / paste in the templates before and may still be there in a few instances. Just my $.02

jreback · 2018-09-18T12:45:02Z

thanks!

use fused types for parts of algos_common_helper

749989f

jbrockmendel mentioned this pull request Aug 21, 2018

POC Use fused types more, tempita less #22432

Closed

gfyoung added Refactor Internal refactoring of code Internals Related to non-user accessible pandas implementation labels Aug 21, 2018

gfyoung approved these changes Aug 21, 2018

View reviewed changes

WillAyd reviewed Aug 21, 2018

View reviewed changes

jreback requested changes Aug 22, 2018

View reviewed changes

jbrockmendel mentioned this pull request Aug 29, 2018

use fused types for reshape #22454

Merged

4 tasks

jbrockmendel added 2 commits September 16, 2018 10:17

Merge branch 'master' of https://github.com/pandas-dev/pandas into temp1

54c6cec

Merge branch 'master' of https://github.com/pandas-dev/pandas into temp1

e25c39a

jreback added this to the 0.24.0 milestone Sep 18, 2018

jreback approved these changes Sep 18, 2018

View reviewed changes

jreback merged commit 8ff8f90 into pandas-dev:master Sep 18, 2018

jbrockmendel deleted the temp1 branch September 18, 2018 13:52

aeltanawy pushed a commit to aeltanawy/pandas that referenced this pull request Sep 20, 2018

use fused types for parts of algos_common_helper (pandas-dev#22452)

c15d8c0

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

use fused types for parts of algos_common_helper (pandas-dev#22452)

4623ef0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use fused types for parts of algos_common_helper #22452

use fused types for parts of algos_common_helper #22452

jbrockmendel commented Aug 21, 2018

gfyoung left a comment

WillAyd Aug 21, 2018

jbrockmendel Aug 21, 2018

WillAyd Aug 21, 2018

WillAyd Aug 21, 2018

jbrockmendel Aug 22, 2018

jreback Aug 22, 2018

scoder Aug 23, 2018 •

edited

Loading

WillAyd Aug 24, 2018

scoder Aug 25, 2018

WillAyd Apr 30, 2019

codecov bot commented Aug 22, 2018 •

edited

Loading

jreback Aug 22, 2018

jreback Aug 22, 2018

jbrockmendel Aug 22, 2018

jreback commented Aug 22, 2018

jbrockmendel commented Aug 22, 2018

jbrockmendel commented Aug 22, 2018

jbrockmendel commented Aug 22, 2018

jbrockmendel commented Sep 7, 2018

WillAyd commented Sep 7, 2018

jreback commented Sep 18, 2018

use fused types for parts of algos_common_helper #22452

use fused types for parts of algos_common_helper #22452

Conversation

jbrockmendel commented Aug 21, 2018

gfyoung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scoder Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 22, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Aug 22, 2018

jbrockmendel commented Aug 22, 2018

jbrockmendel commented Aug 22, 2018

jbrockmendel commented Aug 22, 2018

jbrockmendel commented Sep 7, 2018

WillAyd commented Sep 7, 2018

jreback commented Sep 18, 2018

scoder Aug 23, 2018 •

edited

Loading

codecov bot commented Aug 22, 2018 •

edited

Loading