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

Make datatypes opt-in #22

Closed
ritchie46 opened this issue Feb 9, 2023 · 4 comments · Fixed by #38
Closed

Make datatypes opt-in #22

ritchie46 opened this issue Feb 9, 2023 · 4 comments · Fixed by #38

Comments

@ritchie46
Copy link

Hi @jvdd I am considering adding these to polars, but given that we define our own NaN handling, I only want to expose it for integer backed data.

Could the floating points be feature gated? Polars compile times are really strained so I'd rather not include code we don't need. Let me know what you think!

@jvdd
Copy link
Owner

jvdd commented Feb 9, 2023

Hey @ritchie46, glad to hear that you are considering to include argminmax in polars!

I am certainly open to add a "float" feature (which I would include in the default features). Once PR #21 is finished this should only require adding a single line before the impl_float! macro :)

@ritchie46
Copy link
Author

Great to hear! I will tune back in later then. 👍

@jvdd
Copy link
Owner

jvdd commented Feb 14, 2023

As #21 is +/- finished and actually concerns the NaN handling, I'm quite interested in hearing your opinion on how you believe NaNs should be handled :)

Basically, I see three options:

  • (1) ignore NaNs: return index of highest / lowest non-NaN value (e.g., numpy.nanargmax)
  • return NaNs:
    • (2) use Option<usize> as return type and return None (e.g., numpy.argmax)
    • (3) return index of the first NaN

Before PR #21, the only supported case was option (1) - which was the default behavior.
PR #21 changes the default behavior to option (3), and supports option (1) via the new nanargminmax function.

By quickly scanning the polars code, I believe polars complies with option (2)? Is there any reason why this option is favored over option (3)?

@jvdd
Copy link
Owner

jvdd commented Mar 23, 2023

Hey @ritchie46,

Just released v0.5.0, this release introduces the "float" feature (which is a default feature - for backwards compatibility) and also adds nan-handling (ignoring vs returning).

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 a pull request may close this issue.

2 participants