Better error "Sorted function argument too long"

[jodavies]

Print the required MaxTermSize in words to allow easy configuration of setup.

[coveralls]

coverage: 49.992% (-0.007%) from 49.999% when pulling 89db1dfed12fa789d1fc07192b9bad710b6b45ba on jodavies:sort-info into 83e3d4185efca2e5938c665a6df9d67d6d9492ca on vermaseren:master.

[tueda]

Sounds nice. Possible problems would be:

1. Could it make every sorting in arguments a bit slow? If so, we should compute the required WORD size only when the argument doesn't fit.
2. Maybe you need to consider ErrorMessageLock for the change in Malloc1()? (I'm not sure because anyway Error1() causes the catastrophe #14 via Terminate().)

[vermaseren]

Yes, Terminate is a form of catastrophe. At the moment I am studying what can be done when a term does not fit, such that the job can get control back at for instance at a #enddo or something. I am trying to deal with many cases but quite a few have terms that are too big. I would like to be able to continue with the next case/term. This is however far from trivial. It would have to unwind the workspace and it will never be 100% safe.

The sorting in arguments checks usually in EndSort whether it fits. That check gives (in case of failure) an error message and returns with an error code. It are the calling routines that then decide to call Terminate rather than also returning with a proper error code. It is that ‘unwinding’ that needs to be set up. But to do that brutishly means that the program itself continues as if everything is fine. Hence there has to be a mechanism that restores some state (like at the start of the module (if possible), and lets the user know somehow (per force) that something failed. If then there is some ‘OnFailure’ handling, the program takes that, while if it is absent the program crashes.

Maybe you have some commentary that gives a better scheme?

Jos

On 5 Aug 2018, at 12:33, Takahiro Ueda notifications@github.com wrote:

Sounds nice. Possible problems would be:

Could it make every sorting in arguments a bit slow? If so, we should compute the required WORD size only when the argument doesn't fit. Maybe you need to consider ErrorMessageLock for the change in Malloc1()? (I'm not sure because anyway Error1() causes the catastrophe #14 https://github.com/vermaseren/form/issues/14 via Terminate().) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/vermaseren/form/pull/289#issuecomment-410511025, or mute the thread https://github.com/notifications/unsubscribe-auth/AFLxEoui_7i1zrTr7IdjssWm_-k0aDoxks5uNsncgaJpZM4VvUFK.

[tueda]

Another thing: I don't see any reason why tmpss is declared in the beginning of EndSort. It can be just put in the else block. Physicists and programmers love the principle of locality :-)

[jodavies]

I would imagine that all data is in cache, but I did not do any benchmarks. The new way avoids the problem by only checking if indeed it is too large; also then you do not need tmpss. Declaration wise, I was trying to follow the style of most of the form code.

In tools.c, there are ErrorMessageLock if MALLOCDEBUG is not defined, which is the usual state, no?

[vermaseren]

MALLOCDEBUG is very special for when you are trying to see where you are writing outside allocated memory. It can be very expensive, depending on the options you use. In its most rigorous version it puts some extra allocations before and after the malloced memory and fills it with zeroes. When you free the memory, it checks that there are still only zeroes. This can make things rather slow. Hence it is off by default and only to be used when you become desperate during debugging. Of course nowadays there are other tools that can do similar things, and probably much better. This MALLOCDEBUG is rather old (from the 90’s) but I figured that it is also not a good idea to remove it.

Jos

On 5 Aug 2018, at 18:41, jodavies notifications@github.com wrote:

I would imagine that all data is in cache, but I did not do any benchmarks. The new way avoids the problem by only checking if indeed it is too large; also then you do not need tmpss. Declaration wise, I was trying to follow the style of most of the form code.

In tools.c, there are ErrorMessageLock if MALLOCDEBUG is not defined, which is the usual state, no?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/vermaseren/form/pull/289#issuecomment-410532235, or mute the thread https://github.com/notifications/unsubscribe-auth/AFLxErxLkAl_PFd8iUwZ4FsDi64U7CTEks5uNyA-gaJpZM4VvUFK.

[tueda]

In tools.c, there are ErrorMessageLock if MALLOCDEBUG is not defined, which is the usual state, no?

Ah, you are absolutely right. Somehow I confused between #ifdef MALLOCDEBUG and #ifndef MALLOCDEBUG blocks. As Jos said, it is not the place that emits errors about MaxTermSize and there are many places. But they are beyond of the scope of this patch, so to me it is fine to merge it. (If you like cleanliness, you can git-rebase the two commits.)

@vermaseren and @benruijl, Opinions for Josh's patch? (And what would be a good process to proceed and merge such PRs?)

[jodavies]

With those modifications, I still have some problems with "(1)Output should fit inside a single term. Increase MaxTermSize?". However, none of my messages are printed. This means (and I have verified) that one arrives at this error, not by jumping to the `TooLarge' label, but by taking the if branch here: https://github.com/vermaseren/form/blob/master/sources/sort.c#L963 .

It is not clear to me what causes this, and where I can determine how big the too-large-term is.

I get the error even for very large values of maxtermsize, which does not make any sense to me.

Can one land at this error for other reasons?

Thanks, Josh.

[vermaseren]

Hi,

I ran into such a thing once creating examples for my Form course. The problem is with MaxNumberSize being normally half MaxTermSize. If your polynomials have more normal size coefficients it pays to set MaxNumberSize to ‘reasonable’ values like a few Kilobytes. At least in my case that solved the problem.

Jos

On 23 Aug 2018, at 16:21, jodavies notifications@github.com wrote:

With those modifications, I still have some problems with "(1)Output should fit inside a single term. Increase MaxTermSize?". However, none of my messages are printed. This means (and I have verified) that one arrives at this error, not by jumping to the `TooLarge' label, but by taking the if branch here: https://github.com/vermaseren/form/blob/master/sources/sort.c#L963 https://github.com/vermaseren/form/blob/master/sources/sort.c#L963 .

It is not clear to me what causes this, and where I can determine how big the too-large-term is.

I get the error even for very large values of maxtermsize, which does not make any sense to me.

Can one land at this error for other reasons?

Thanks, Josh.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vermaseren/form/pull/289#issuecomment-415432136, or mute the thread https://github.com/notifications/unsubscribe-auth/AFLxEo6hoijWqj57eVa6fYILy3SYCOUrks5uTrqAgaJpZM4VvUFK.

[vsht]

I think that a better error message here would be very useful. The current one only creates unnecessary confusion, as in #538

[jodavies]

With those modifications, I still have some problems with "(1)Output should fit inside a single term. Increase MaxTermSize?". However, none of my messages are printed. This means (and I have verified) that one arrives at this error, not by jumping to the `TooLarge' label, but by taking the if branch here: https://github.com/vermaseren/form/blob/master/sources/sort.c#L963 .

It is not clear to me what causes this, and where I can determine how big the too-large-term is.

I get the error even for very large values of maxtermsize, which does not make any sense to me.

Can one land at this error for other reasons?

Thanks, Josh.

These cases are related to #515 , which added some relevant commentary. From here it is not possible to print a useful error message stating the maxtermsize which would be required though. The way to do that would be to let FORM continue and fail later, instead of terminating "early".