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

kernel: don't use ProdIntObj as ProdFunc; use PowObjInt less #2198

Merged
merged 1 commit into from
Feb 23, 2018

Conversation

fingolfin
Copy link
Member

We used to install ProdIntObj and PowObjInt for a large variety of TNUMs,
including precords, functions, strings etc., where they never made sense:

And for those TNUMs where it did make sense (e.g. T_RAT), often later modules
would install more specialized functions anyway. For ProdIntObj, this meant
that the only sensible place it was used was for T_MACFLOAT. This PR removes
even that, though, since it was only used for int * float, but not for
float * int, which resulted in the two behaving differently:

gap> 10^400 * 10.^-300;
1.e+100
gap> 10.^-300 * 10^400;
inf

Hence, we stop using ProdIntObj as kernel level ProdFunc completely, even for
machine floats, and instead,rely on library functions for multiplying integers
and floats.

Note that ProdIntObj can still be invoked from the library via PROD_INT_OBJ,
and this is used to install default multiplication functions for integers with
IsNearAdditiveElementWithInverse elements (and these method installations are
also provided symmetrically, i.e. intobj as well as objint). While this is
very slightly slower, it is at least guaranteed to be symmetric and
mathematically well-defined.

For PowObjInt, the asymmetry is natural, so less of a problem. Also, it turns
out that it is actually used in a few more cases, e.g. to power
transformations and partial permutations. Still, doing f^1 for a function or
'x'^1 or true^1 are all surprising. To resolve this, we restrict for which
TNUMs we install PowObjInt as PowFunc substantially: We don't use it on all
constant TNUMs anymore, but rather only on those were multiplication by an
integer makes some sense, using the new TNUM range FIRST_MULT_TNUM till
LAST_MULT_TNUM. We also don't install it for records anymore, and also not for
blists and strings. It is kept for ranges and other plists, as there it has a
chance of a well-defined behavior (if the list describes a vector,
possibly a matrix).

We used to install ProdIntObj and PowObjInt for a large variety of TNUMs,
including precords, functions, strings etc., where they never made sense:

And for those TNUMs where it did make sense (e.g. T_RAT), often later modules
would install more specialized functions anyway. For ProdIntObj, this meant
that the only sensible place it was used was for T_MACFLOAT. This PR removes
even that, though, since it was only used for `int * float`, but not for
`float * int`, which resulted in the two behaving differently:

    gap> 10^400 * 10.^-300;
    1.e+100
    gap> 10.^-300 * 10^400;
    inf

Hence, we stop using ProdIntObj as kernel level ProdFunc completely, even for
machine floats, and instead,rely on library functions for multiplying integers
and floats.

Note that ProdIntObj can still be invoked from the library via PROD_INT_OBJ,
and this is used to install default multiplication functions for integers with
IsNearAdditiveElementWithInverse elements (and these method installations are
also provided symmetrically, i.e. int*obj as well as obj*int). While this is
very slightly slower, it is at least guaranteed to be symmetric and
mathematically well-defined.

For PowObjInt, the asymmetry is natural, so less of a problem. Also, it turns
out that it is actually used in a few more cases, e.g. to power
transformations and partial permutations. Still, doing f^1 for a function or
'x'^1 or true^1 are all surprising. To resolve this, we restrict for which
TNUMs we install PowObjInt as PowFunc substantially: We don't use it on all
constant TNUMs anymore, but rather only on those were multiplication by an
integer makes some sense, using the new TNUM range FIRST_MULT_TNUM till
LAST_MULT_TNUM. We also don't install it for records anymore, and also not for
blists and strings. It is kept for ranges and other plists, as there it has a
chance of a well-defined behavior (if the list describes a vector,
possibly a matrix).
@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them topic: kernel labels Feb 21, 2018
@fingolfin
Copy link
Member Author

fingolfin commented Feb 21, 2018

Some examples of things GAP accepted before this PR (but which trigger an error with it):

gap> 1 * Immutable(rec());
rec(  )
gap> rec() ^ 1;
rec(  )
gap> 1 * 'x';
'x'

@ChrisJefferson
Copy link
Contributor

We should watch for this commit when we run package tests, just in case some package is doing something stupid.

@fingolfin fingolfin merged commit e94dfa6 into gap-system:master Feb 23, 2018
@fingolfin fingolfin deleted the mh/strict-prodint branch February 23, 2018 11:56
@fingolfin fingolfin added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Mar 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Issues describing general bugs, and PRs fixing them release notes: added PRs introducing changes that have since been mentioned in the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants