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

Support as_int on bool series #2359

Merged
merged 6 commits into from
Jan 17, 2025
Merged

Support as_int on bool series #2359

merged 6 commits into from
Jan 17, 2025

Conversation

rebkwok
Copy link
Contributor

@rebkwok rebkwok commented Jan 16, 2025

Fixes #1313

  • support using is_int on a bool series
  • add spec test
  • Update trino engine (has its own custom cast_to_int method to round floats towards zero, which isn't applicable for booleans)
  • Replace the case statement for casting bools to ints in the measures framework

Copy link
Contributor

@madwort madwort left a comment

Choose a reason for hiding this comment

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

Code looks good, although because it's so end-user focused I'd like to see some more documentation - e.g. it should definitely be mentioned in https://docs.opensafely.org/ehrql/reference/features/#7-operations-on-boolean-series & maybe also as an alternative option in https://docs.opensafely.org/ehrql/reference/features/#1113-case-with-boolean-column ?

@rebkwok
Copy link
Contributor Author

rebkwok commented Jan 17, 2025

Code looks good, although because it's so end-user focused I'd like to see some more documentation - e.g. it should definitely be mentioned in https://docs.opensafely.org/ehrql/reference/features/#7-operations-on-boolean-series & maybe also as an alternative option in https://docs.opensafely.org/ehrql/reference/features/#1113-case-with-boolean-column ?

Thanks, I'd forgotten to add the new spec test file to the toc.py so the reference docs weren't being pulled in. Also added a comment in the case spec test.

Copy link

Deploying databuilder-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: e4e4ce0
Status: ✅  Deploy successful!
Preview URL: https://fd92a278.databuilder.pages.dev
Branch Preview URL: https://cast-bool-to-int.databuilder.pages.dev

View logs

@madwort
Copy link
Contributor

madwort commented Jan 17, 2025

wow, not sure I'd really seen that test/docs tooling, that's brill (& the docs look much better now thanks)

@rebkwok rebkwok merged commit 6673e53 into main Jan 17, 2025
8 checks passed
@rebkwok rebkwok deleted the cast-bool-to-int branch January 17, 2025 13:16
Copy link
Contributor

@evansd evansd left a comment

Choose a reason for hiding this comment

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

Great, thanks for doing this!

One other thing it might be worth doing (non-urgently) is extending this bit of the generative tests so it produces examples of bools being cast to ints:

@st.composite
def cast_to_int(draw, type_, frame):
type_ = draw(any_numeric_type())
return Function.CastToInt(draw(series(type_, frame)))

We've got code in the tests which will moan at you if you add a completely new operation and don't include it in the generative tests. But we don't have anything which reminds you to update the tests if you extend an existing operation (I don't quite know how we'd do that to be honest) so it's one of those things which we inevitably forget.

@@ -376,7 +376,7 @@ class FloorDivide(Series[int]):

# Casting numeric types
class CastToInt(Series[int]):
source: Series[Numeric]
source: Series[Numeric] | Series[bool]
Copy link
Contributor

Choose a reason for hiding this comment

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

You could save eight whole characters by writing this as:

Series[Numeric | bool]

Eight, Becky! What were you thinking?

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.

Expand CastToInt operation to accept Series[bool]
3 participants