-
Notifications
You must be signed in to change notification settings - Fork 145
Add Series.index_of #1118
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
Add Series.index_of #1118
Conversation
billylanchantin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this fell off my radar.
Thanks for the addition! Seems quite handy.
I'm not super happy with the inconsistent error messages for mismatching types but if there's a better way I'm happy to change it.
Did you have any more ideas?
One thought is to try and make a singleton series first in Elixir with the known dtype. If that fails because of a dtype mismatch, then we can fail with a consistent error message. If not, we can pass along both series. As a bonus, we won't need to do any additional casting in Rust.
So something like this:
def index_of(series, value) do
value_series =
try do
from_list([value], series.dtype)
catch
# ...
end
apply_series(series, :index_of, [value_series])
endIf this approach doesn't work well, then I think the mismatched error messages are acceptable (if not ideal).
b3f2b80 to
a2bad72
Compare
|
Sorry I didn't see the notification till now and thanks for your suggestion that's a much nicer solution. I found implementing it results in odd behaviour for series of type duration as it does not cast values to different precision like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent! I agree this solution is much nicer.
I've requested one additional comment to explain the {:duration, _} thing, then this is good to go.
|
Excellent work, @fizzlebert! Thank you for the addition! |
| # Struct | ||
| @callback field(s, String.t()) :: s | ||
|
|
||
| @callback index_of(s, valid_types()) :: integer() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realised this should have a return type of integer() | nil not just integer(). I can make a new pr for it if you think it's necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No that's ok, I'll just change it. Thanks for letting us know!
I wanted to use the index_of in elixir so here's my implementation. My rust skills are lacking but I tried, so please let me know if anything needs fixing. I'm not super happy with the inconsistent error messages for mismatching types but if there's a better way I'm happy to change it.