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

discuss: top-level APIs future #7147

Closed
1 task done
NickCrews opened this issue Sep 13, 2023 · 12 comments
Closed
1 task done

discuss: top-level APIs future #7147

NickCrews opened this issue Sep 13, 2023 · 12 comments
Labels
deprecation Issues or PRs related to deprecating APIs ux User experience related issues

Comments

@NickCrews
Copy link
Contributor

NickCrews commented Sep 13, 2023

What happened?

Thought of these things as I'm going through moving top-level stuff in #7141

  • [ibis.greatest](https://ibis-project.org/reference/top_level#ibis.expr.api.greatest) has no docstring. [ibis.ifelse](https://ibis-project.org/reference/top_level#ibis.expr.api.ifelse) has an incorrect docstring. Should we make some more refined mechanism that updates/fixes the docstrings to be correct (ie replace use of "self", add an additional param in front)? Or just keep it simple and copy paste, with a few manual edits?
  • Do we even need top-level ibis.ifelse? when is that needed/preferable to BooleanValue.ifelse()? Just trying to reduce our API. Similar with .isin, name, and a few others.
  • Can we deprecate ibis.where? ifelse does the same thing (IMO with a better name), and I'd prefer there to be exactly one way. Maybe just a soft deprecation where we remove it from the docs but don't add a DeprecationWarning?

What version of ibis are you using?

main

What backend(s) are you using, if any?

Na

Relevant log output

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@NickCrews NickCrews added the bug Incorrect behavior inside of ibis label Sep 13, 2023
@contang0
Copy link

I can only agree that gravitation towards a smaller API is a virtue! I like it that with Ibis I usually can think of only one correct way to do a task, and this has to do with absence of redundant / overly similar methods.

@cpcloud
Copy link
Member

cpcloud commented Sep 14, 2023

Should we make some more refined mechanism that updates/fixes the docstrings to be correct (ie replace use of "self", add an additional param in front)? Or just keep it simple and copy paste, with a few manual edits?

Probably a copy-paste is fine for these cases.

Do we even need top-level ibis.ifelse? when is that needed/preferable to BooleanValue.ifelse()? Just trying to reduce our API. Similar with .isin, name, and a few others.

I think the top-level is around for folks coming from tools and systems that are function-based (SQL, R/dplyr) instead of class/method-based tools like those in the PyData ecosystem.

I'm in favor of reducing our API surface by removing redundancies, though in this particular case the API is literally a copy of the other.

Any chance you'd be will to do an audit for redundancies and post it here for discussion 😅?

Can we deprecate ibis.where?

Yeah, I think that makes sense. The soft deprecation sounds like a fine approach to me.

@NickCrews
Copy link
Contributor Author

Any chance you'd be will to do an audit for redundancies and post it here for discussion 😅?

I think the .where/.ifelse is the only redundancy I can think of at this point. Will bring them up here if I find them as I refactor.

The soft deprecation sounds like a fine approach to me.

One question is should we rename ops.Where to ops.IfElse? The name of computed columns is "Where", which is a little confusing to users. But that has always been an inconsistency you saw if you used .ifelse, so this isn't a new problem we are creating.

@cpcloud
Copy link
Member

cpcloud commented Sep 14, 2023

ibis.expr.operations is not guaranteed to be stable across non-major versions, so renaming ops.Whereops.IfElse seems fine to me.

@NickCrews
Copy link
Contributor Author

Ah, one other instance of aliasing the same thing is ibis.NA and ibis.null(). Sort weird one is a function the other an attribute. I would love to move to just one of these, but IDK which. I like how NA is shorter, but null seems to be the more general/accepted term. I would like to be able to specify a type for the null, ie ibis.null("string") (that isn't currently supported, I have to do ibis.literal(None, "string"), but in the future I would want to add this), so keeping the API as a function seems good.

@cpcloud
Copy link
Member

cpcloud commented Sep 14, 2023

Yeah, there's sort of a question whether NullLiteral is worth keeping around internally too.

It's not clear that literal(None, T) wouldn't be strictly better than NullLiteral.

@cpcloud
Copy link
Member

cpcloud commented Sep 14, 2023

Let's deal with these on a case by case basis for now and avoid any general decisions until we see a pattern emerge.

NickCrews added a commit to NickCrews/ibis that referenced this issue Sep 14, 2023
NickCrews added a commit to NickCrews/ibis that referenced this issue Sep 14, 2023
@NickCrews
Copy link
Contributor Author

NickCrews commented Sep 18, 2023

related (in the sense of trying to reduce the API surface): Can we remove (or merely remove from docs as a start)

  • NumericValue.zeroifnull(): easily replaced with .fillna(0)
  • NumericValue.nullifzero(): easily replaced with Value.nullif(0)

NickCrews added a commit to NickCrews/ibis that referenced this issue Sep 18, 2023
NickCrews added a commit to NickCrews/ibis that referenced this issue Sep 20, 2023
@NickCrews
Copy link
Contributor Author

NickCrews commented Sep 20, 2023

exhaustive list of null/NA inconsistencies/reduncancies (✅ means it exists, ❌ means it doesn't)

  • ✅ isnull
  • ❌ isna
  • ✅ notnull
  • ❌ notna
  • ✅ nullif
  • ❌ naif (how am I supposed to pronounce that, "nafe"??? 🤣 )
  • ✅ fillna
  • ❌ fillnull
  • ✅ ibis.NA
  • ✅ ibis.null

It seems like we slightly already prefer null over NA. I agree with this, I think NULL is more generally accepted.

What do we think about soft-deprecating fillna and ibis.NA (no warning, but removing from docs/examples), and moving to just NULL?

@jcrist
Copy link
Member

jcrist commented Sep 20, 2023

What do we think about soft-deprecating fillna and ibis.NA (no warning, but removing from docs/examples), and moving to just NULL?

We've talked about this in the past with respect to fillna/dropna, I don't think that's a bad idea. Some users may assume those operations work with nan too, which is incorrect.

I personally never use ibis.NA and don't fully understand when it's needed so maybe that could be deprecated too?

NickCrews added a commit to NickCrews/ibis that referenced this issue Sep 21, 2023
NickCrews added a commit to NickCrews/ibis that referenced this issue Sep 22, 2023
@cpcloud cpcloud removed the bug Incorrect behavior inside of ibis label Sep 26, 2023
@cpcloud cpcloud changed the title bug: discuss: top-level APIs future discuss: top-level APIs future Sep 26, 2023
@cpcloud cpcloud added ux User experience related issues deprecation Issues or PRs related to deprecating APIs labels Sep 26, 2023
NickCrews added a commit to NickCrews/ibis that referenced this issue Sep 27, 2023
cpcloud pushed a commit that referenced this issue Sep 27, 2023
@NickCrews
Copy link
Contributor Author

I personally never use ibis.NA

I also don't understand the distinction. I use it interchangeably/inconsistently with ibis.null().

Once #7262 is done I think I'm going to submit a PR moving to null from na, as I outline above.

@jcrist
Copy link
Member

jcrist commented Oct 2, 2023

With #7263 and #7236 in, I think we can close this. The remaining na-related APIs are covered in #7276.

@jcrist jcrist closed this as completed Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation Issues or PRs related to deprecating APIs ux User experience related issues
Projects
None yet
Development

No branches or pull requests

4 participants