Skip to content
This repository was archived by the owner on Feb 2, 2024. It is now read-only.

Scale Series.median(skipna=True/False)#579

Closed
PokhodenkoSA wants to merge 1 commit intoIntelPython:masterfrom
PokhodenkoSA:scale-median
Closed

Scale Series.median(skipna=True/False)#579
PokhodenkoSA wants to merge 1 commit intoIntelPython:masterfrom
PokhodenkoSA:scale-median

Conversation

@PokhodenkoSA
Copy link
Copy Markdown
Contributor

@PokhodenkoSA PokhodenkoSA commented Feb 6, 2020

image
Bad performance.


def nanmedian_impl(a):
# Create a temporary workspace with only non-NaN values
temp_arry = np.empty(a.size, a.dtype)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Result of median always should be float

def np_nanmedian(a):
if not isinstance(a, types.Array):
return
isnan = get_isnan(a.dtype)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good. Let's use it everywhere

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

After some investigation, i see few issues with it:

  1. It return False for ints, but the function itself doesn't inline
  2. Even if it would inline, there is no dead code elimination pass after inline pass. Which means, the condition would not be eliminated
  3. Even if dead code elimination would be added in previous versions it works only with conditions like if True: and if False: and doesn't work with conditions like if not False.

Probably llvm would be able to fix all this issues in it's own IR. Probably not. Need to check and measure

Still, let's stick to the strategy use this function

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants