Add std.atan2 #1119
Add std.atan2 #1119
Conversation
| @@ -109,6 +109,7 @@ local html = import 'html.libsonnet'; | |||
| <ul><code>std.asin(x)</code></ul> | |||
| <ul><code>std.acos(x)</code></ul> | |||
| <ul><code>std.atan(x)</code></ul> | |||
| <ul><code>std.atan2(y, x)</code></ul> | |||
There was a problem hiding this comment.
Not sure about this part, it should probably go to the rest of std functions, because it was not-existant, yet rest of the math functions are defined here, because they are builtins.
Although it won't be used very often, so it's okay to skip availableSince discoverability?
There was a problem hiding this comment.
Agree with the location, but IMHO we should include a note mentioning when it was added. It doesn't have to be put into the availableSince field, I think we can just include a parenthetical note in the text on this line. e.g.,
<ul><code>std.atan2(y, x)</code> (available since 0.21.0)</ul>
There are some other places (e.g., functions only available in Go) where that information is just in text in the description, not in the structured data, so I think just including version in the description text here is fine.
There was a problem hiding this comment.
Hi! Thanks for your patience on this. This seems like a good thing to add!
I would personally prefer this to be implemented as a builtin using libc atan2, primarily because floating point calculations can have tricky boundary conditions and precision issues (not even considering the weird numbers like NaNs and infinities), and I'm not expert enough to assess whether there are any such problems here. I mean, this implementation is probably totally fine, but even quite simple functions like hypot can be tricky to implement robustly.
What's your opinion on this?
I do see that putting it into std.jsonnet has the advantage of covering the Go jsonnet implementation as well.
I see you already put a builtin implementation into jrsonnet (CertainLach/jrsonnet@a5b2c19)
| @@ -109,6 +109,7 @@ local html = import 'html.libsonnet'; | |||
| <ul><code>std.asin(x)</code></ul> | |||
| <ul><code>std.acos(x)</code></ul> | |||
| <ul><code>std.atan(x)</code></ul> | |||
| <ul><code>std.atan2(y, x)</code></ul> | |||
There was a problem hiding this comment.
Agree with the location, but IMHO we should include a note mentioning when it was added. It doesn't have to be put into the availableSince field, I think we can just include a parenthetical note in the text on this line. e.g.,
<ul><code>std.atan2(y, x)</code> (available since 0.21.0)</ul>
There are some other places (e.g., functions only available in Go) where that information is just in text in the description, not in the structured data, so I think just including version in the description text here is fine.
|
@CertainLach Any response to my previous comment? How would you feel about implementing this as a builtin instead? |
Fixes: #1118