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

CLN: revisit build warnings in cython templates #27346

Merged
merged 2 commits into from Jul 12, 2019
Merged

CLN: revisit build warnings in cython templates #27346

merged 2 commits into from Jul 12, 2019

Conversation

ghost
Copy link

@ghost ghost commented Jul 11, 2019

  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

Retrying dropped portions from #27157, which amend cython templates, to deal with special-cases based on type. In this Pr, compiler warnings about x==x being useless in the case of ints, but not floats (since nan!=nan).

In #27157 (comment) these were rejected because there wasn't agreement about how this should done and a separate discussion was requested. Annoyingly, these changes agree with what already exists in these files (in multiple places):

{{if name == 'int64'}}
mask = (masked_vals == {{nan_val}}).astype(np.uint8)
{{else}}
mask = np.isnan(masked_vals).astype(np.uint8)
{{endif}}

But the file is already inconsistent in how these type checks are done:

if groupby_t is int64_t:

Making a decision here also required to get rid of the warnings mentioned in #27169, in that case fused types which mix signed/unsigned trigger warning when the code later includes comparisons with a signed quantity. So there too, type checks in the template are the obvious fix.

cc @WillAyd, @jreback

@WillAyd
Copy link
Member

WillAyd commented Jul 11, 2019

I'm not really sure this is an improvement as it just adds more layers to the templating where we already have a weird mix between templates and fused types. Is there a way to just do this all using fused types perhaps?

@jreback
Copy link
Contributor

jreback commented Jul 11, 2019

actually i think these changes are ok, though can you have a look and see how consistent we are with these idioms across cython? is there a way to centralize these templates (easily)? e.g. have a is_null type of template

@jreback jreback added the Clean label Jul 11, 2019
@ghost
Copy link
Author

ghost commented Jul 11, 2019

is there a way to centralize these templates (easily)?

maybe, I'll try it. But then, what would to do with the signed/unsigned compares in #27157?

@jreback jreback added this to the 0.25.0 milestone Jul 12, 2019
@jreback
Copy link
Contributor

jreback commented Jul 12, 2019

ok this is positive progress towards reducing warnings.

@jreback jreback merged commit 07d0488 into pandas-dev:master Jul 12, 2019
@ghost ghost deleted the cython_template_warnings branch July 31, 2019 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants