Project

General

Profile

Bug #946

Function "ideal" evaluates the argument twice

Added by Anna Maria Bigatti over 7 years ago. Updated about 2 years ago.

Status:
Resolved
Priority:
Urgent
Category:
Parser/Interpreter
Target version:
Start date:
17 Oct 2016
Due date:
% Done:

80%

Estimated time:
6.00 h
Spent time:

Description

ideal(RING, func(...)) evaluates func once,
ideal(func(...)) evaluates func twice.


Related issues

Related to CoCoA-5 - Slug #31: theValue makes copyIn Progress2011-11-15

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

From looking at the code in 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 list
  • gcd of a list of int or list of ring elem
  • lcm (like gcd)
  • apply where 2nd arg is a list
  • ContentWRT where 2nd arg is a list
  • CoefficientsWRT where 2nd arg is a list
  • ElimMat where 1st is a list, or where 2nd arg is an INT
  • ideal
  • NewFreeModuleForSyz where arg is a list
  • submodule where 1st arg is a list
  • elim where 1st arg is a list
  • PolyAlgebraHom where 3rd arg is a list
  • PolyRingHom 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

Also available in: Atom PDF