Fixed quotient and modulus for non-integer domains #3400
Conversation
| scalarProduct(RootSystem,Weight,Weight) := (R,u,v) -> | ||
| ( | ||
| sum apply(apply(entries((R.CartanMatrixTrInv)*u),entries(v),(x,y)->x*y),R.RootNorms,(x,y)->x*y)//2 | ||
| sum apply(apply(entries((R.CartanMatrixTrInv)*u),entries(v),(x,y)->x*y),R.RootNorms,(x,y)->x*y) / 2 |
There was a problem hiding this comment.
@baptistecalmes could you take a look at this change, please?
I'm trying to clarify the difference between QQ // ZZ and QQ / ZZ in Macaulay2.
For instance, until now (35/2) // 2 gave 35/4, but to make // behave uniformly, I'd like to change it to (35/2) // 2 = floor(35/4) = 8. This changed caused an example in the package HighestWeights to fail, and I tracked it down to this division here.
Should this division result in a rational or integer number? Was there a reason // was used rather than /? Also, there are a couple of more uses of // 2 a few lines above and below here. Should those also be changed to / 2?
There was a problem hiding this comment.
Actually, looking at the following tests, I think the rational number result was intended in all three cases, so I'll go ahead and change those as well.
assert(scalarProduct(rootSystemA(2),1,2)==1/6)
R=rootSystemA(2);
v=weight(R,{1,2});
assert(scalarProduct(R,1,v)==2/3)
R=rootSystemA(2)
u=weight(R,{1,2})
v=weight(R,{0,3})
assert(scalarProduct(R,u,v)==5/2)I appreciate your thorough test coverage!
There was a problem hiding this comment.
@mahrud I'm sure the coherent behavior of // that you are trying to reach has been discussed among core developers of M2, but I personally do not quite get what it's supposed to be outside of Euclidean rings or maybe slight generalization of them. In particular, it's unclear to me that QQ // QQ should return anything at all, and if it does, I assume it should be an integer. Then, I do not see how general this behavior should be. What should be the output ring when // is defined on some ring?
Relatedly, concerning your changes to WeylGroups, could you please add test cases for various definitions of eval ensuring that eval always returns an integer, just in case // is modified further in the smoothing process, since you've removed the floor function in the code that used to ensure that.
There was a problem hiding this comment.
I initiated the discussion about this in #3340.
I will add more tests for eval!
There was a problem hiding this comment.
Actually, it might be simpler to leave floor there as is.
mahrud
force-pushed
the
quickfix/reals
branch
3 times, most recently
from
e7f0096
to
004fd42
Compare
Example: RR_100[x]; x/2
Closes #3340.