Skip to content

Conversation

@lidavidm
Copy link
Member

No description provided.

@github-actions
Copy link

@ianmcook
Copy link
Member

Thanks for doing this @lidavidm! You mind if I push a commit to your fork to tweak some of the R stuff?

@lidavidm
Copy link
Member Author

Yeah, please go ahead! I admit I'm not familiar with the R side of things and mostly just pattern match on similar code.

@ianmcook
Copy link
Member

The fact that is_inf returns false for NaN is inconvenient because its negation returns true.

Both R and NumPy have separate functions to test for finite and invite values, and these all return false for NaN:

> is.infinite(NaN)
[1] FALSE
> is.finite(NaN)
[1] FALSE
>>> np.isinf(np.nan)
False
>>> np.isfinite(np.nan)
False

I wonder if this suggests we should also make an is_fin or is_finite kernel to avoid this problem of negation.

@lidavidm
Copy link
Member Author

Oh, doh. Yes, then having an is_finite kernel would make more sense than just inverting it. I'll update that.

@ianmcook
Copy link
Member

This also fixes an inconsistency with the is.na() method for Scalar objects in the R package. Previously that returned a length-1 R logical vector. Now it returns a Scalar having the bool data type. This was necessary go make is.finite() and is.infinite() work consistently.

Copy link
Contributor

@cyb70289 cyb70289 left a comment

Choose a reason for hiding this comment

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

LGTM, +1

@cyb70289
Copy link
Contributor

@ianmcook , do you have other comments? Can we merge this PR?

@ianmcook
Copy link
Member

@cyb70289 No more comments from me, looks good, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants