Opened 7 years ago

Last modified 5 years ago

#164 new defect - FPE

grid command causes fpe

Reported by: Gary J. Ferland Owned by: nobody
Priority: major Milestone:
Component: input parser Version: trunk
Keywords: Cc:

Description

both attempted uses of the grid command for the bernard loop paper ended in fpes. the common thread is R3244:3245


the following throws an fpe

test element nitrogen abundance -4.2 vary grid -4.5 -3.5 0.5

adding log to the command does not help. The grid command does what it is supposed to do - it emits -4.5, -4. and -3.5. The problem is the forced linear interpretation of these negative numbers at parse_elements.cpp:484 - added in 3245


the following throws an fpe test constant temperature 4 vary grid from 4e3K to 2e4K in 2e3K steps

the problem is r3244 - forced log interpretation at parse_constant.cpp:65, which is forced by the log on the vary option at line 100. that forced log is r4344

every grid command i tried for this paper threw an fpe.

Change History (5)

comment:1 Changed 7 years ago by Gary J. Ferland

Ryan pointed out that the log/linear keyword on the grid command would fix the constant temperature case - that is correct. But the problems are wider and deeper than only that.

Simplicity is always better than complexity. A possible solution: it introduces another level of complexity - lots of commands accept the vary/grid option. would all commands require the linear / log option? for commands that don't have the linear/log distinction, do we still require the keyword on the grid command? or is it only required for temperature commands?

i have not studied this part of the code - does the linear / log option change what the grid command emits? svn blame shows that the current source for the constant temperature log interpretation had a delta on r3941/3942 on 2010 may 27 - comments say there was no net change with this pair. the forced log interpretation was added to the vary command in r3244. the command had followed the code's rules before that rev.

if we agree that we want to move towards "no logs in the input parser" we don't want to add more log use, and also want to make it possible to not use them at all. I don't remember why we force the log rule in this constant temperature command, or what inspired r3244.

We need to do cases where the grid command emits the following for quantities that range over a vary wide range

hden 3 vary grid 1 5 1

then do hden 1 hden 2 hden 3 hden 4 hden 5

for this case we do want log steps.

but for other cases (like what i was trying to do) we want linear steps, as the following:

constant temperature 3 grid 4e3 6e3 1e3

we want to do: constant temperature 4e4 constant temperature 5e4 constant temperature 6e4

I think both would work if we just had the simplest possible grid command which emitted what a for loop would do in C. It should do nothing clever at all. Then let the constant temperature command follow the code's other rules. Is there something wrong with this simplest possible rule? There must have been a good reason for r3244

We will have to think about how to do the first case when we don't have logs, but the current implementation has that problem too.

My question, before adding more rules to the grid command, is there a problem with a really simple grid command, which simply emitted what a C "for" loop would do, with the constant temperature following the code's rules?

comment:2 Changed 7 years ago by Gary J. Ferland

there is a hack to get the constant temperature command to not throw an fpe in r4098

comment:3 Changed 7 years ago by Gary J. Ferland

one final note -

user input must never be blindly accepted. If a number has been scanned in THE CODE WILL CHECK THAT IT IS >0 BEFORE TAKING A LOG. Anything else is irresponsible.

user input should be taken as a dangerous assault on the code. Verify it before accepting it. Do not throw an fpe. Explain what is wrong. It should be impossible for a number entered on a command line to cause an fpe.

comment:4 Changed 7 years ago by Gary J. Ferland

r4112 hacks the element name abundance command so that it works with the grid command

comment:5 Changed 5 years ago by Ryan Porter

Milestone: C10 release
Priority: blockermajor

Works fine now. Removing the milestone and downgrading from blocker to major, because there are likely still some many similar log/linear issues.

Note: See TracTickets for help on using tickets.