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

Add atan support for missings #43523

Merged
merged 7 commits into from
Dec 27, 2021
Merged

Add atan support for missings #43523

merged 7 commits into from
Dec 27, 2021

Conversation

nicholasbalasus
Copy link
Contributor

allowing atan to deal with two missing arguments

allowing atan to deal with two missing arguments
@oscardssmith oscardssmith changed the title Update math.jl Add atan support for missings Dec 22, 2021
@oscardssmith oscardssmith added maths Mathematical functions missing data Base.missing and related functionality labels Dec 22, 2021
@oscardssmith
Copy link
Member

This seems reasonable given that we already define atan(missing).

@garrison
Copy link
Member

Thank you for the pull request (the first ever submitted from your account, according to GitHub!).

  • Is there any reason not to have it return missing as well if only one of the provided arguments is missing?
  • Might it be worth adding test(s)?

@oscardssmith oscardssmith added the needs tests Unit tests are required for this change label Dec 23, 2021
base/math.jl Outdated
@@ -1318,6 +1318,7 @@ for f in (:sin, :cos, :tan, :asin, :atan, :acos,
end
@eval $(f)(::Missing) = missing
end
atan(::Missing,::Missing) = missing
Copy link
Member

@garrison garrison Dec 23, 2021

Choose a reason for hiding this comment

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

This line would be more consistent with the rest of the julia code base if a space were added after the comma. (Although there are a handful of exceptions, the space is present much more often than it is not.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, garrison! I have just added support for the cases where one of the values is missing and the other is not. I believe ::Number is better for this than ::Any but am not sure!

atan(::Missing, ::Missing) = missing
atan(::Number, ::Missing) = missing
atan(::Missing, ::Number) = missing

Copy link
Member

Choose a reason for hiding this comment

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

I agree that ::Number makes the most sense, as it makes this consistent with other two-argument math functions (e.g. *, /).

Added space after the commas. Added cases for one of the values being missing.
@garrison
Copy link
Member

garrison commented Dec 23, 2021

I think the easiest (and, in my opinion, the most appropriate) place to add a test case would be to add it to the list of "arithmetic operators". If this path is chosen, I think the phrase "arithmetic operators" should perhaps be renamed to "binary operations" (i.e., within the variable name, testset name, and comments). Note that the single argument form of atan is tested later in the same file, under "elementary functions".

EDIT: Actually, I believe it makes better sense to start a testset for "two-argument functions" right below "elementary functions." Others for which missing support could be implemented in the future include log and hypot. I'll leave the links to the "arithmetic operator" tests above because that's a good example of what to emulate.

@nicholasbalasus
Copy link
Contributor Author

Added now (and it looks like the checks passed now after I had quite a few issues with the white space bot, my apologies!)

as it’s written now, I left atan in the “elementary functions” test set as well for the one input test, though this could definitely be changed if it would be better practice to only have it in one place.

@ViralBShah ViralBShah merged commit c790938 into JuliaLang:master Dec 27, 2021
garrison added a commit to garrison/julia that referenced this pull request Dec 30, 2021
This is in the spirit of JuliaLang#43523, which added `missing` support
for the two-argument form of `atan`.
garrison added a commit to garrison/julia that referenced this pull request Dec 30, 2021
This is in the spirit of JuliaLang#43523, which added `missing` support
for the two-argument form of `atan`.
garrison added a commit to garrison/julia that referenced this pull request Jan 5, 2022
This is in the spirit of JuliaLang#43523, which added `missing` support
for the two-argument form of `atan`.
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
Allowing atan to deal with two missing arguments
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Allowing atan to deal with two missing arguments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maths Mathematical functions missing data Base.missing and related functionality needs tests Unit tests are required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants