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

Only check isnan if applicable #54

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ianfiske
Copy link

@ianfiske
Copy link
Author

As per the discussion in #53, I added a check if isnan is applicable before calling it. Note that now, the tests fail because there is also no isfinite method for Millisecond. I can fix this with the same strategy of checking if the method is applicable, but this just feels clunky to me. I'm used to the "call the method" and it does the right thing for each type. Checking before calling so many methods just starts to look clunky, but if you think that's the right approach, I can easily add it.

@nalimilan
Copy link
Member

I agree it's not ideal, but the only other solution I can see is to define isnan(::Any) and isfinite(::Any) to be false. Do you feel like proposing this in Base? That would probably allow making the code cleaner.

CI errors, but AFAICT it's no related to isfinite?

@nalimilan
Copy link
Member

Bump.

@ianfiske
Copy link
Author

ianfiske commented Nov 9, 2020

@nalimilan Thanks for the bump, I've been busy with other things and I don't think I have time to make the PR to Base right now. Feel free to run with it if you have time before I do.

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.

Cannot compute quantile(x::Array{Millisecond,1})
2 participants