MAINT: Refactor logic in maybe_convert_* #15018

gfyoung · 2016-12-30T23:14:36Z

Creates a Seen class to abstract objects encountered when doing type conversions.

Follow-up to #15005.

jreback · 2016-12-30T23:31:41Z

you need to pass the structure to the functions themselves

but i think a class is better here actually

gfyoung · 2016-12-31T01:47:21Z

@jreback : Good catch regarding classes. Fixed.

jreback · 2016-12-31T01:59:33Z

pandas/src/inference.pyx

@@ -629,23 +676,31 @@ cdef int64_t iINT64_MIN = <int64_t> INT64_MIN
 cdef uint64_t iUINT64_MAX = <uint64_t> UINT64_MAX


-cdef inline bint _check_uint64_nan(bint seen_uint, bint seen_null,
-                                   bint coerce_numeric) except -1:
+cdef inline bint _check_uint64_nan(Seen seen) except -1:
    """


so this should be a method of Seen

jreback · 2016-12-31T01:59:41Z

pandas/src/inference.pyx

@@ -654,22 +709,30 @@ cdef inline bint _check_uint64_nan(bint seen_uint, bint seen_null,
    return False


-cdef inline bint _check_uint64_int64_conflict(bint seen_sint, bint seen_uint,
-                                              bint coerce_numeric) except -1:
+cdef inline bint _check_uint64_int64_conflict(Seen seen) except -1:


jreback · 2016-12-31T02:01:31Z

pandas/src/inference.pyx

-                                 coerce_numeric):
-                return values
-
+            seen.seen_null = True


I would make each of these cases where you are currently setting a method of Seen instead

e.g.

def saw_float(self): self.seen_nul = True self.seen_float = True

jreback · 2016-12-31T02:02:23Z

pandas/src/inference.pyx

-                _check_uint64_int64_conflict(seen_sint, seen_uint,
-                                             coerce_numeric)):
-                return values
+            seen.seen_int = True


def saw_int(self, int val): self.seen_int = True ...

this way the loop is pretty simple then.

jreback · 2016-12-31T02:31:18Z

pandas/src/inference.pyx

+    """
+
+    cdef public:
+        bint seen_int


I would also change these variables to something like

_bool _null _uint

makes reading much easier w/o all the extra seen (alternatively sbool, snull, suint, sint, sfloat.....)

codecov-io · 2016-12-31T09:39:32Z

Current coverage is 84.77% (diff: 100%)

Merging #15018 into master will not change coverage

@@             master     #15018   diff @@
==========================================
  Files           145        145          
  Lines         51129      51129          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits          43344      43344          
  Misses         7785       7785          
  Partials          0          0

Powered by Codecov. Last update 9947a99...531a361

gfyoung · 2016-12-31T09:41:04Z

@jreback : Addressed all comments, and everything is green still.

jreback · 2016-12-31T15:35:52Z

pandas/src/inference.pyx

+
+        return False
+
+    cdef inline saw_null(self):


I think, and maybe you have a better refence. that it doesn't matter if you cdef a method in a cdef class, by-definiiion they are always cdef. e.g. in Timestamp we mark these def actually.

I also am not sure of the inline when in a cdef class (IOW it may or may not make any difference).

I don't know if it is necessary either, but I do see cdef (cdef inline in the codebase) for cdef class methods (e.g. Textreader). I don't think it could hurt to be explicit.

Adding inline is a performance thing, and couldn't hurt here since we aren't doing anything complicated in this function.

jreback · 2016-12-31T15:42:38Z

pandas/src/inference.pyx

+
+        return False
+
+    cdef inline bint check_uint64_int64_conflict(self) except -1:


I believe you are now calling both of these routines check_uint64..... always together with an or?

if so, any reason to combine them?

That makes perfect sense. Done.

gfyoung · 2016-12-31T21:11:18Z

@jreback : Made the requested changes, and everything is green.

jreback · 2017-01-01T18:10:19Z

pandas/src/inference.pyx


+    if not seen.sobject:
        if not safe:


dont' you need:

if seen.check_uint64_conflict(): somewhere here?

No actually because when we detect conflicts, seen.sobject is set to 1, so that whole branch of logic is skipped.

jorisvandenbossche · 2017-01-02T09:29:21Z

Small question (and sorry that you already changed all those once before), but can you name it seen.int, seen.null, etc instead of see.sint, seen.snull, .. ?
IMO that reads easier (and sint and snull actually mean other things, or at least in Dutch :-))

gfyoung · 2017-01-02T09:31:18Z

@jorisvandenbossche : Fair enough (what do they mean, just curious?). With underscores (as @jreback suggested) or none? I'm inclined to go with no underscores since not declaring them public makes it pretty clear I think that they are private but want to double check.

jorisvandenbossche · 2017-01-02T09:35:42Z

underscores are not needed here I think (if we add an underscore, I would make the class _Seen (the instantation can still be seen)

(sint -> saint, snul(l) -> nitwit/squib (don't know the exact translation))

gfyoung · 2017-01-02T09:36:37Z

@jorisvandenbossche : Very well. Can rename without underscores.

jreback · 2017-01-02T12:12:39Z

i don't think u can name a struct member the same as its type e.g.

int int

is not valid code (python or c)

jreback · 2017-01-02T12:14:08Z

i see the renaming - that is ok

lgtm then

jreback · 2017-01-02T15:40:05Z

pandas/src/inference.pyx

            break

-    seen_numeric = seen_complex or seen_float or seen_int
+    seen.numeric_ = seen.complex_ or seen.float_ or seen.int_


seen.numeric_ should be a property in Seen

jreback · 2017-01-02T15:42:40Z

pandas/src/inference.pyx

-                if not seen_bool and not seen_datetime and not seen_timedelta:
-                    if seen_complex:
+            if seen.null_:
+                if (not seen.bool_ and not seen.datetime_


it would be less code here if you would make this compound if as a property to Seen, though only if it has an intutie name, maybe is_float_or_complex (you might also need to pass safe to the Seen constructor

jreback · 2017-01-02T15:43:05Z

pandas/src/inference.pyx

-                    if seen_complex:
-                        if not seen_int:
+            if seen.null_:
+                if (not seen.bool_ and not seen.datetime_


you could use the same is_float_or_complex here as well

jreback · 2017-01-02T15:44:31Z

@gfyoung my last 2 comments are meant, not really to simplify the code much, but more to make it a bit easier to read w/o having to think thru all of the possible cases.

Creates a Seen class to abstract objects encountered when doing type conversions.

gfyoung · 2017-01-02T19:38:26Z

@jreback : Certainly. There was another similar refactoring that I did for is_bool too.

jreback · 2017-01-02T19:44:53Z

lgtm. just give a quick perf test (locally) and ping on green.

gfyoung · 2017-01-03T00:21:33Z

@jreback : No major perf impact, and everything is green. Ready to merge if there are no other concerns.

jreback · 2017-01-03T11:25:00Z

thanks!

gfyoung force-pushed the maybe-convert-refactor branch from e4fa8f4 to b74baac Compare December 31, 2016 01:30

jreback reviewed Dec 31, 2016

View reviewed changes

jreback added Clean Dtype Conversions Unexpected or buggy dtype conversions labels Dec 31, 2016

jreback reviewed Dec 31, 2016

View reviewed changes

gfyoung force-pushed the maybe-convert-refactor branch from b74baac to 0bd265c Compare December 31, 2016 05:56

jreback reviewed Dec 31, 2016

View reviewed changes

gfyoung force-pushed the maybe-convert-refactor branch from 0bd265c to 2f696a5 Compare December 31, 2016 19:22

jreback reviewed Jan 1, 2017

View reviewed changes

jreback added this to the 0.20.0 milestone Jan 1, 2017

gfyoung force-pushed the maybe-convert-refactor branch from 2f696a5 to c62bb1d Compare January 2, 2017 11:27

jreback reviewed Jan 2, 2017

View reviewed changes

MAINT: Refactor logic in maybe_convert_*

531a361

Creates a Seen class to abstract objects encountered when doing type conversions.

gfyoung force-pushed the maybe-convert-refactor branch from c62bb1d to 531a361 Compare January 2, 2017 19:37

jreback closed this in 136a6fb Jan 3, 2017

gfyoung deleted the maybe-convert-refactor branch January 3, 2017 18:22


		return False

		cdef inline bint check_uint64_int64_conflict(self) except -1:

MAINT: Refactor logic in maybe_convert_* #15018

MAINT: Refactor logic in maybe_convert_* #15018

Conversation

gfyoung commented Dec 30, 2016 • edited Loading

jreback commented Dec 30, 2016

gfyoung commented Dec 31, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Dec 31, 2016 • edited Loading

Current coverage is 84.77% (diff: 100%)

gfyoung commented Dec 31, 2016

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

gfyoung commented Dec 31, 2016

Choose a reason for hiding this comment

gfyoung Jan 1, 2017 • edited Loading

Choose a reason for hiding this comment

jorisvandenbossche commented Jan 2, 2017

gfyoung commented Jan 2, 2017

jorisvandenbossche commented Jan 2, 2017

gfyoung commented Jan 2, 2017

jreback commented Jan 2, 2017

jreback commented Jan 2, 2017

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

jreback commented Jan 2, 2017

gfyoung commented Jan 2, 2017

jreback commented Jan 2, 2017

gfyoung commented Jan 3, 2017

jreback commented Jan 3, 2017

gfyoung commented Dec 30, 2016 •

edited

Loading

codecov-io commented Dec 31, 2016 •

edited

Loading

gfyoung Jan 1, 2017 •

edited

Loading