Math module - unifying argument names #19005
Comments
|
I would need to see the complete list. Let's sort out the default and non-default names first. But generally a good idea. While some people use Even so, I tend to use 'u' .. 'z' or even 'f' .. 'h' for anything of real(w) type, and 'i' .. 'n' for things integral. I thought that Exactly what you do with say max and min which apply to both integral and real(w) types, I do not have a perfect answer. |
|
I feel disinclined to have the argument names try to encode the types of the arguments, or at least would need to hear a better argument for why this is a good/valuable direction. Specifically, I'm not excited about trying to encode types into identifiers in general (preferring to rely on the documentation, code, or tools to provide that information) and wouldn't want to set an expectation that specific argument names suggest specific types that might apply to other modules beyond Math. |
|
Prep summary for an ad-hoc subteam discussion, anticipated for starting the week of July 11th, 2023. 1. What is the API being presented?I've done a pass over all the functions in the Math and AutoMath modules as of this writing. Here is the summary of the argument names that are used and how widely: Math:
Two args:
AutoMath:
Two args:
Three args (var args):
Four args:
How is it being used in Arkouda and CHAMPS?Arkouda:
CHAMPS:
Especially for the single argument case, it seems likely that users are not relying on named arguments to call these functions, so changing the argument names will hopefully not be too burdensome of a change. 2. What's the history of the feature, if it already exists?Math is one of the earliest modules. Not all symbols were added immediately, but they were added in a relatively short span of time as compared to how long this module has been in existence. We recently shuffled symbols around to impact what was made available to all Chapel programs by default. Most of what is provided is based on symbols available in C, although isclose was inspired by Python. It does not seem like we followed C's argument naming convention when we added them (though my information is based on C++'s references to the C functions as of today, rather than looking at the pages from the original date they were added, so the argument names may have changed). 3. What's the precedent in other languages, if they support it? As relevant, we especially care about:a. Python Quick summary: because Python doesn't care as much about type strictness, it does not have argument names that vary based on the type that is provided nor multiple overloads from the same function. My information was taken from the math package. All one arg functions in the math package that we care about in the context of this issue use b. C/C++ Quick summary: C/C++ is the least consistent on the names that it uses of the languages that I surveyed and I wasn't always sure about the reason for using one name or the other. My information was taken from this reference page The most common name for the single argument case was c. Rust Quick summary: My information is mostly taken from the f64 page, though I did quickly check the f32 page and didn't initially spot any differences. Rust defines these functions as methods on their first argument, so all of the first arguments are named d. Swift Quick summary: I was able to find most of these functions on the Core Graphics Functions page, which a stackoverflow post linked to, so my information is taken from there. That location seems a little unintuitive to me, so if there's a better place I should be looking, I'm all ears! For the single argument case, these functions use e. Julia Quick summary: I took this information from Julia's base math page. The handling for their rounding functions is not as easily comparable to ours so I haven't included it directly in this data, but the curious can take a look at it in this page. There is some variance in argument names otherwise, but it is mostly unified. The most common argument name in the single argument case is f. Go Quick summary: I got this information from Go's math package. The most common name in the single argument case is 4. Are there known Github issues with the feature?The most relevant known Github issue is #19025, which talks about Because this is covering almost the entire Math and AutoMath modules, there's a lot of side issues related to the functions themselves rather than the argument names specifically. I'll list them here for completeness, but they're mostly a distraction:
5. Are there features it is related to?The naming of operator arguments issue is related but less important because there's no way for the user to use the argument names when calling the operator. We might feel the need to rename arguments to similar functions in the BigInteger module. 6. How do you propose to solve the problem?A. Leave things as is
B. All i, r, z, im, val arguments get turned into
C. Ditto B, but use
D. Ditto B/C but reverse the order of the
E. Ditto B/C but rename
Other combinations or ideas welcome. |
|
Upon looking at |
|
Summary:
|
|
We also decided that BigInteger will have similar renamings |
|
What type does each of What has Chapel traditionally called a complex argument? Ditto for an imag type |
It varies. But the intention is that it will use a unified name across the types.
There are not very many places where Chapel has explicitly complex or imag arguments. The main other place aside from the Math/AutoMath modules is Another point to consider when thinking about this is the difference between explicit overloads for the various types and defining a single method with a generic argument. In the latter case, we get the same name for all types by default. We generally find this beneficial, allowing the user to not worry about the type being used as long as the function supports that type. It wouldn't be beneficial in the case of When we have inconsistencies in how things are handled across modules, it makes it easy for users to notice patterns that were never intended. It's fiddly and annoying to make changes that feel so minor, but in the long run, it will help the language feel more intentional and polished, and give users an actual foundation for pattern recognition. |
Adds a divide by zero check to `BigInteger.mod`, `BigInteger.%` and `BigInteger.%=` to match the divide functions (#19384). While doing this work, implemented part of the change for argument names (#19005 (comment)). ## Summary of changes - add a divide by zero check to `mod` - reimplement `mod` in terms of the `divR` with the proper rounding argument - add helper methods for `modTrunc` by a `integral` to reduce code duplication - deprecate argument names `a`/`b` on `mod` in favor of `x`/`y` ## New tests - adds `test/library/standard/BigInteger/modByZero.chpl` - adds `test/deprecated/BigInteger/argumentNames.chpl` ## Testing - paratest with futures - paratest + gasnet - built and checked docs [Reviewed by @bmcdonald3] closes #19384 closes Cray/chapel-private#5094
|
#22837 will resolve 3 of the five decisions. Will handle |
[reviewed by @jabraham17] We decided in a recent discussion that we wanted to fix the argument names in the Math and AutoMath modules so that they were consistent with each other and most other modules. This means that: - All `i`, `r`, `z`, `im`, and `val` arguments will get turned into `x`, including `val` in `logBasePow2`. - All `m`, `n` and `a`, `b` pairs will get transformed into `x`, `y` pairs. - `logBasePow2` will also change its `baseLog2` argument to `exp` Implements part of the decision in #19005. Deprecates all the arguments that would need to be changed to comply with this decision in the AutoMath module (except in functions that are already deprecated due to moving to the Math module). Deprecates the arguments in `logBasePow2` because it was moved more than 1 release ago. Note: this PR does not add deprecation messages for the functions that were recently moved to the Math module so they would no longer be included by default. This is because doing so results in resolution conflicts with the deprecations in AutoMath: we want to maintain deprecations for two releases and it seems unlikely that any of these functions are being called with named arguments in the first place, due to only having a single argument (in the case of things like `cos`) or the argument ordering not mattering (in the case of `gcd`). The only exception is `ldexp`, which may get renamed entirely as a result of discussion in #19021 and so will be put off until we know the result of that discussion (because then the version with the old arguments won't need to be marked as "last resort" to generate the deprecation message and so won't conflict with the version in AutoMath) Note: also does not rename the arguments to `isclose`. This is because `isclose` itself is also getting renamed, so will do that at the same time in a separate PR. Removes the old deprecation for `logBasePow2` since it has been deprecated for a few releases and leaving it would cause conflicts. In doing so, left the helper function implementing it in place due to the deprecated version of `log2` relying on it (and added a note to those functions to ensure it will get cleaned up when they are removed). Adds deprecation tests when deprecations were added. Only two tests ended up needing modifications as a result of this change - the submitted mandelbrots now have a slightly different deprecation message due to extending the message for divceil/etc, which we stopped including by default in the current release cycle. Passed a full paratest with futures
Renames remaining BigInteger arguments to `x` and `y`, unless they imparted a special meaning of their functionality (like `n` or `numer`). This unifies argument names for differing types in overloaded functions. Implements changes from #19005 (comment) ## Summary of changes - change arguments of `init(num)` and `init(str)` to `init(x)` - also changed `init=`, but that should not be a user facing change - change arguments of `root(result, a, b)` to `root(result, x, y)` - change arguments of `sqrt(result, a)` to `sqrt(result, x)` - change arguments of `invert(result, a)` to `invert(result, x)` - change arguments of `add(result, a, b)` to `add(result, x, y)` - change arguments of `sub(result, a, b)` to `sub(result, x, y)` - change arguments of `mul(result, a, b)` to `mul(result, x, y)` - change arguments of `neg(result, a)` to `neg(result, x)` - change arguments of `abs(result, a)` to `abs(result, x)` - change arguments of `cmp(b)` to `cmp(x)` - change arguments of `cmpabs(b)` to `cmp(x)` - change arguments of `and(result, a, b)` to `and(result, x, y)` - change arguments of `com(result, a)` to `com(result, x)` - change arguments of `xor(result, a, b)` to `xor(result, x, y)` - change arguments of `set(num)`, `set(a)`, and `set(str)` to `set(x)` - change arguments of `swap(a)` to `swap(x)` - updates and extends documentation for all affected functions ## New tests - extends `test/deprecated/BigInteger/argumentNames.chpl` ## Testing - [x] paratest with futures - [x] paratest + gasnet - [x] built and checked docs [Reviewed by @bmcdonald3] closes #22161 closes Cray/chapel-private#5100
[reviewed by @jabraham17] In recent discussions, we decided to: - rename `ldexp` to `ldExp` - rename its `n` argument to `exp` This will make the function name easier to understand (the word boundaries are much more clear with camelCasing) and more accurately describe the argument. Resolves #19005 Resolves #19021 Updated the Random module to use the new version instead of the deprecated one. Updated the deprecation message in AutoMath to mention that the version in Math has a different name now. Updated the test tracking that error as well Updated the doc link reference to use the new name. Added a test of the deprecation message. Updated a GPU test that was using the old name. Passed a full paratest with futures
Argument name for a lot of the functions vary depending on the type of the argument - I see
ifor integer,xfor real andzfor complex but not always. I think these argument names should be unified (probably tox?).There are three categories of functions that use a different strategy than the above. I think they should also be unified, but it seemed worth calling out how they are different in case they should stay as exceptions:
divceil, etc usemandnfor their argument names. Should it bexandyinstead? They at least seem to be consistent with each other.modalso mostly uses the same names, but not always.logusesxfor the argument name except for thelog2variants which useval. These should be unified (tox, I believe)aandbfor its argument names. Should it usexandy?The text was updated successfully, but these errors were encountered: