CoCoA » CoCoALib

John Abbott wrote:

Making InvMod throw an exception is an interface change, so it could in principle break some existing code (but is there any outside CoCoALib?).

There might be. We don't know... but that's because documentation is too good ;-)
I heard about a cocoalib user I never met (that reminds me i had to check his feedback and comments)

In this case I think that the "backward compatibility" aspect is of relatively little importance.
If someone has used that aspect of (old) InvMod, and assuming we do offer InvModNoThrow then it is enough just to change the name of the fn being called (or change the code to catch ERR::DivByZero).

It seems to be too risky to allow the "standard" function InvMod to return a wrong answer "without warning" (i.e. throwing an exception).

The actual implementations in NumTheory.C do throw rather than return 0 -- I do not know when the code was changed.

Perhaps I'll just update the documentation, and wait and see if anyone complains?

Thinking about it: I would prefer to have 0 if it is relatively costly to determine if it is divisible.

We also have, in general

  bool IsDivisible(RingElem& lhs, ConstRefRingElem num, ConstRefRingElem den)
  bool IsDivisible(ConstRefRingElem num, ConstRefRingElem den)

In QuotientRing.C there is

 virtual bool myDivMod(RingElemRawPtr rawlhs, RingElemConstRawPtr rawnum, RingElemConstRawPtr rawden) const;

The documentation was already updated to say that ERR:DivByZero is thrown if the inverse does not exist.

Note that the inverse exists iff gcd(value,modulus)==1. Computing the GCD should be slightly less costly than calling InvMod(value,modulus); the algorithms are virtually the same (but InvMod has to compute the values of a "cofactor" which will eventually give the final answer).

Another possible interface is an optional 3rd arg to say not to throw: e.g. InvMod(res,mod,NoThrow). This avoids having to invent a new fn name; there will be a (tiny?) run-time cost in case the inverse does not exist (since the decision about what to do is made at run-time).

In any case, the internal implementation probably would be neatest if it did not throw "internally"; so just the publicly visible fns would potentially throw.
NOTE this is not what the impl currently does... sigh!

John Abbott wrote:

Another possible interface is an optional 3rd arg to say not to throw: e.g. InvMod(res,mod,NoThrow). This avoids having to invent a new name; there will be a (tiny?) run-time cost in case the inverse does not exist (since the decision about what to do is made at run-time).

I like that! and we can use it in similar cases.
Maybe we could have the NoThrow arg to be an enum with just one choice (usable in all CoCoALib), and therefore with no run-time cost. This is essentially identical to implementing InvModNoThrow(res,mod), but for some reason I have a preference for InvMod with two signatures.

In any case, the internal implementation probably would be neatest if it did not throw "internally"; so just the publicly visible fns would potentially throw.
NOTE this is not what the impl currently does... sigh!

• InvMod(r,m) throws if the inverse does not exist
• InvMod(r,m, flag) never throws; if the inverse does not exist, it returns 0 and sets flag (which is a bool&)

The order of the args in the second option might be different.

A possible disadvantage here is that the flag is of type bool (but we could make it a different type, perhaps an enum). With bool it is not clear to me if true means "no problem", or if it means "there was a problem".

Perhaps the NoThrow arg as suggested in comment 9 is clearer?
Comments? Opinions?

John Abbott wrote:

Perhaps the NoThrow arg as suggested in comment 9 is clearer?

clearer

I have implemented enum ErrorActionIndicator { ThrowOnError, NoThrow };

And the functions now have a 3rd arg const ErrorActionIndicator ErrorAction = ThrowOnError.

To be honest the name of the enum is excessively long.
Also when an error is encountered the test to be made is something like if (ErrorAction == ThrowOnError). This is again rather long.

Should I aim for a more compact solution?

I'd rather have (as I said in comment #10)

NoThrowMarker { NoThrow };

@Anna: your approach is quite possible but it is more awkward than what I have implemented.

My approach has an optional 3rd arg (i.e. always present but with a default value); this means that I just had to modify each of the 5 signatures (and the corresponding impls slightly).

Your approach requires me to add 5 new signatures (for a total of 10); also the impls become slightly more indirect. To avoid duplicating code, the public fns would presumably call the "real" fns which actually do the work with a "boolean" flag saying what to do in case of error.

In summary: it can be done your way, but I cannot currently see any neat way of implementing it.

John Abbott wrote:

Your approach requires me to add 5 new signatures (for a total of 10);

good point!

I have just checked in my impl. Perhaps Anna could have a look at it, and decide whether she thinks it is comprehensible enough.

I have done some tidying in NumTheory.C (e.g. removing useless arg checks).
Everything compiles, and all tests pass here :-)

Currently the enum ErrorActionIndicator is defined inside NumTheory.H; probably it should be moved to utils.H so other files can use it...?

John Abbott wrote:

Currently the enum ErrorActionIndicator is defined inside NumTheory.H;

May we call it enum ThrowFlag {Throw, NoThrow}?

probably it should be moved to utils.H so other files can use it...?

yes!

checked-out, compiles, run, comprehensible enough.

I have just thought of an argument in favour of Anna's proposed design (namely, different signatures).
In C++11 there is a noexcept specifier which can be used to specify that a function does not throw. With Anna's proposal this can be used for the "NoThrow" versions of the fns; with my proposal it cannot be used at all.

I don't know what this really means in practice; supposedly noexcept functions can run a bit faster, and perhaps produce smaller compiled code.

I wonder what we should do?

• (A) different fn names: InvMod(r,m) and InvModNoThrow(r,M)
• (B) extra arg: InvMod(r,m) and InvMod(r,m,NoThrow)

To me, approach (A) seems slightly simpler (but the user has to remember an extra function name, though it ought to be easy to remember). The name could be slightly different: InvMod_NoThrow.

Approach (B) might encourage us to be coherent if we need throwing and non-throwing versions of several functions -- can you think of any other candidate fns?

I wonder which approach would be easiest to document, or perhaps there is no real difference?

Perhaps the best approach is not to worry about noexcept at the moment since we have no experience of the feature. We could note this issue as one to be revisited when we switch to C++11, and are familiar with its features.

What do you think?

Is the current CVS version acceptable for the time being?

I am still a bit perplexed by this issue.

Having InvMod always throw seems to me to be the cleanest and safest solution.

An advantage of the non-throwing version is that a caller can easily then choose to throw its own exception: for instance InvMod is called inside PowerMod. Currently this calls the throwing version of InvMod, so the exception will report that it was thrown by InvMod; a previous version of PowerMod checked the return value of InvMod, and would throw if there was a problem -- the exception would then say that it was thrown by PowerMod. Is this difference important?

When I have to debug code which unexpectedly threw an exception, I normally set a break-point inside CoCoA::ThrowError and then "walk up the stack"; so it would make little difference which routine actually threw.

Undecided :-/