Bug #946
Function "ideal" evaluates the argument twice
Description
ideal(RING, func(...))
evaluates func once, ideal(func(...))
evaluates func twice.
Related issues
History
#1 Updated by John Abbott over 7 years ago
- Status changed from New to In Progress
- Priority changed from High to Urgent
- % Done changed from 0 to 10
Having just looked at the code, I'm not entirely surprised.
Also why are there calls to evalArgAsListOfRingElem
and evalArgAsListOf<RingElem>
? :-/
It seems to me that the arg has already been evaluated in the call to evalArgAsT1orT2orT3
, and saved in the variable x
.
#2 Updated by John Abbott over 7 years ago
Two test cases:
define f(x) println "Inside f"; return x; enddefine; I1 := ideal(f(x)); --> BAD I2 := ideal([f(x)]); --> BAD I3 := ideal(R, [f(x)]);--> OK
#3 Updated by John Abbott over 7 years ago
Here is some code we could add to one of the CoCoA-5 tests:
use R ::= QQ[x]; define f(x) TopLevel FLAG; if FLAG then error("Called f twice"); endif; FLAG := true; return x; enddefine; -- f FLAG := false; I1 := ideal(f(x)); FLAG := false; I2 := ideal([f(x)]); FLAG := false; I3 := ideal(R, [f(x)]);
Probably the name of the top-level variable should be a bit longer (and more unusual).
The test prints nothing if all is well, otherwise a CoCoA error is reported.
#4 Updated by John Abbott over 7 years ago
Are there any other fns with a similar problem? 8-{
#5 Updated by John Abbott over 7 years ago
Oh dear! The problem is more widespread. Even len
evaluates its arg twice 8-O
#6 Updated by John Abbott over 7 years ago
BuiltinFunctions-CoCoALib.C
, here is a list of other suspect impls:
len
syz
(of a list)SyzOfGens
just what is this code supposed to do???homog
where first arg is a listgcd
of a list of int or list of ring elemlcm
(likegcd
)apply
where 2nd arg is a listContentWRT
where 2nd arg is a listCoefficientsWRT
where 2nd arg is a listElimMat
where 1st is a list, or where 2nd arg is an INTideal
NewFreeModuleForSyz
where arg is a listsubmodule
where 1st arg is a listelim
where 1st arg is a listPolyAlgebraHom
where 3rd arg is a listPolyRingHom
where 4rd arg is a list
JAA thinks these are all the cases from BuiltinFunctions-CoCoALib.C
#7 Updated by John Abbott about 7 years ago
For an urgent issue, this is making very slow progress :-(
Perhaps someone (who??) should produce a test file which covers all the fns listed in comment 6 above.
Then someone (who??) can try cleaning up the code... any volunteers?
#8 Updated by Anna Maria Bigatti about 7 years ago
I think this is far too difficult for this release.
I suggest moving it to next.
#9 Updated by John Abbott about 7 years ago
- Target version changed from CoCoA-5.2.0 spring 2017 to CoCoA-5.2.2
Despite the urgent status I have postponed this to the next version. After all it is principally a matter of efficiency rather than correctness. Moreover, a simple workaround should be just to put values in variables and pass the variables to the affected functions: evaluating a variable twice (rather than just once) should be barely noticeable.
#10 Updated by Anna Maria Bigatti almost 7 years ago
- Status changed from In Progress to Resolved
- Assignee set to Anna Maria Bigatti
- Estimated time changed from 3.00 h to 6.00 h
Found it! (and fixed most of them)
The problem is when using the hand functions EvalArgAsT1OrT2..<..>(ARG(0), which)
: this returns a RightValue
which is then (usually) converted with RefTo<class>
.
Unfortunately sometimes it is not as simple as this and EvalArgAs<...>(ARG(0))
is called again on the same argument, and, as the name says, EvalArgAs..
evaluates the argument again.
One common case is for a list of RingElem, so I now implemented evalRVAsListOfRingElem(RightVal, ARG(0))
(RV is for RightValue) which takes the already processed RightVal, and ARG only for indicating errors, in case.
For other functions I just made sure ARG is not used twice, but there are still a few cases (a funny case in gcd, for example)
#11 Updated by Anna Maria Bigatti almost 7 years ago
- % Done changed from 10 to 70
#12 Updated by Anna Maria Bigatti almost 7 years ago
I made a test: "bug-EvalTwice.cocoa5" where we can collect all cases we meet, solved or unsolved.
#13 Updated by John Abbott over 6 years ago
- Target version changed from CoCoA-5.2.2 to CoCoA-5.2.4
Postponing to next version; it is better to deal with all fns, and then close (rather than close after having dealt with just some of the fns).
#14 Updated by John Abbott over 5 years ago
- Target version changed from CoCoA-5.2.4 to CoCoA-5.3.0
#15 Updated by John Abbott over 4 years ago
- Target version changed from CoCoA-5.3.0 to CoCoA-5.4.0
#16 Updated by John Abbott over 4 years ago
- Target version changed from CoCoA-5.4.0 to CoCoA-5.3.0
I have just run the test given in comment 2; now only the first one evals its arg twice.
Is this easy to fix?
Moving target back to 5.3.0. We should at least produce a test suite for the fns listed in comment 6, so we know how much progress we are making (and how far we are from finishing). If the test suite shows that we are far from finishing then we can change the target back to 5.3.2.
#17 Updated by John Abbott about 4 years ago
- Target version changed from CoCoA-5.3.0 to CoCoA-5.4.0
#18 Updated by John Abbott about 4 years ago
Maybe the move ctor from C++14 can help here?
Postponed because it will take (a lot of) time to fix.
#19 Updated by John Abbott about 3 years ago
I might have fixed this (for ideals)... and probably introduced some new weird bugs.... the interpreter code is not easy to fathom :-(
#20 Updated by John Abbott about 3 years ago
I have just checked in my revised code (even though it still needs to be cleaned).
We must develop a proper test suite for the various cases listed in comment 6. This will take some time; I hope most cases have already been resolved, but an objective test would give a nice confirmation.
#21 Updated by Anna Maria Bigatti almost 3 years ago
We must develop a proper test suite for the various cases listed in comment 6. This will take some time; I hope most cases have already been resolved, but an objective test would give a nice confirmation.
See #946-12
"I made a test: "bug-EvalTwice.cocoa5" where we can collect all cases we meet, solved or unsolved."
#22 Updated by Anna Maria Bigatti over 2 years ago
at the end of this file there is a section "TO BE FIXED" with the last few cases to be fixed.
Anna Maria Bigatti wrote:
We must develop a proper test suite for the various cases listed in comment 6. This will take some time; I hope most cases have already been resolved, but an objective test would give a nice confirmation.
See #946-12
"I made a test: "bug-EvalTwice.cocoa5" where we can collect all cases we meet, solved or unsolved."
#23 Updated by John Abbott over 2 years ago
Created new fn SpecializeToListOfRingElem
which converts a generic LIST
into a list of RINGELEM
(I hope).
See Interpreter.H
#24 Updated by John Abbott over 2 years ago
- % Done changed from 70 to 80
Just 1 left: ModuleElem
Relevant source seems to be in BuiltinFunctions-CoCoALib.C
around line 1465.
At the moment, I have no idea why the arg is evaluated twice... the source code looks sane.
#25 Updated by Anna Maria Bigatti over 2 years ago
(I'll see ModuleElem)
#26 Updated by Anna Maria Bigatti over 2 years ago
Anna Maria Bigatti wrote:
(I'll see ModuleElem)
Worked together: ModuleElem is fine. The problem is in submodule (see ideal)
#27 Updated by John Abbott over 2 years ago
Should check all fns which call evalArgAsT1or
My computer/fgrep says there are 85 such fns.
#28 Updated by Anna Maria Bigatti over 2 years ago
Fixed PolyRingHom and PolyAlgebraHom.
Added evalRVAsListOfRingElem
with ring
CVS'd
#29 Updated by John Abbott about 2 years ago
Can we close this issue? If not, postpone it!
#30 Updated by Anna Maria Bigatti about 2 years ago
fixed evalArgAsListOfSymbols, updated test (NewPolyRing
)
#31 Updated by Anna Maria Bigatti about 2 years ago
- Target version changed from CoCoA-5.4.0 to CoCoA-5.4.2
Mostly done, enough for CoCoA-5.4.0.
Still to be fixed submodule
(see bug-EvalTwice.cocoa5
).
Probably it is the last one missing.
#32 Updated by John Abbott about 1 year ago
- Related to Slug #31: theValue makes copy added