Skip to content
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

Coercion fixes #54

Merged
merged 3 commits into from
Jun 16, 2016
Merged

Coercion fixes #54

merged 3 commits into from
Jun 16, 2016

Conversation

akx
Copy link
Contributor

@akx akx commented Jun 16, 2016

Closes #43 (rebased and augmented version)

},
},
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Care to explain in the commit message why this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Come to think of it, this shouldn't really be in this PR at all. My previously misconfigured tox setup installed 1.10a1, which failed miserably.

@akx akx force-pushed the coercion-fixes branch 2 times, most recently from 88cbc7b to 33116b0 Compare June 16, 2016 07:57
return None
if not isinstance(value, self.enum):
return self.enum(value).value
return value.value # Already the correct type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this better than the original (which is shorter)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, the modified version is better, because it's more explicit about the data types which could be thrown in...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this would still be better the other way around:

if value is None:
    return None
elif isinstance(value, self.enum):
    return value.value
return self.enum(value).value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's approximately 15% faster on average in my benchmark.

Somewhat slower when you pass in a value that needs to be casted (1 below), but a lot faster when you pass in a value that's already the correct type (E1.A below).

get_prep_value_patch(E1.A): 1.267213796993019
get_prep_value_typecheck(E1.A): 0.719976721011335
get_prep_value_patch(1) 1.785125483002048
get_prep_value_typecheck(1): 1.8604830430122092

Aggregate results:

get_prep_value_typecheck: 2.7982757450372446
get_prep_value_patch: 3.270842725993134

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhat slower when you pass in a value that needs to be casted (1 below), but a lot faster when you pass in a value that's already the correct type (E1.A below).

Which is the more common case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance testing a single function out of context doesn't tell much, since like in this case, if it's faster for some cases and slower for some others, then it's hard to judge which version is be better overall. Anyway, as I said, the longer version is more explicit and therefore easier to understand and that's enough for me.

Copy link
Contributor Author

@akx akx Jun 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@suutari-ai: I instrumented my venv's django-enumfields to keep track of which code path it used and ran the test suite for shuup/shuup@abcdcd4 (nice hash!):

prep_value benchmark: [('nocast', 114)]

I think it's safe to say the no-cast case is somewhat more common 😆

EDIT: using shuup/shuup@09d7e2f:

prep_value benchmark: [('nocast', 247)]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, nocast is ran 247 times. How many times is it ran with the cast?

Copy link
Contributor Author

@akx akx Jun 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zero times! :D

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. sounds like there could just as well be assert isinstance.

As said in the documentation of get_prep_value [1]:

value is the current value of the model’s attribute, and the method should return data in a format that has been prepared for use as a parameter in a query.

So how could model's attribute be something else than None or instance of the enum? I don't think it can.

[1] https://docs.djangoproject.com/en/1.9/ref/models/fields/#django.db.models.Field.get_prep_value

akx and others added 3 commits June 16, 2016 11:15
This seems to be approximately 15% faster on average than naively
re-casting everything. (For values that need to be cast, it's 5% slower.
For values that need not be cast, it's 75% faster.)

Augments ed7d1d9

Refs hzdg#48
This breaks when you have two enum values with the same label.
@akx
Copy link
Contributor Author

akx commented Jun 16, 2016

@suutari-ai I flipped if not isinstance to if isinstance, as you suggested. It's a nicer way to put it, agreed.

@suutari-ai
Copy link
Contributor

LGTM

@akx akx merged commit 3b87bea into hzdg:master Jun 16, 2016
@akx akx deleted the coercion-fixes branch June 16, 2016 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants