Ticket #661 (closed defect: fixed)

Opened 4 weeks ago

Last modified 3 weeks ago

binder_ew potential problem with nearest_quake_dist option

Reported by: paulf Owned by: paulf
Priority: major Milestone:
Component: binder_ew Version:
Keywords: Cc:

Description

Jin-Koo Lee found another potential problem:

I found something in bind_ew.
I do not know if it's a bug.

I think that it is necessary to reset npix in grid.c


from grid.c and assume i=457(=lQuake)
928	if (is_quake_simultaneous(i)) {
929             pQuake[i].npix = 0;	---> ADD THIS ONE.
930		goto nada;	/* terminate nucleation, too close to other quake */
931	}


Because......

If not reset npix = 0,

from bind.c and assume l1=357, l2=456. mQuake=100, lQuake=457
565	for(l=l1; l<l2; l++) {
566		iq = l % mQuake;   ----> iq = 57, it also means 457!!!
567		if(pQuake[iq].npix == 0) /* Killed event? */    ----> if not reset in grid.c than npix>0
568			continue;
569		tdif = pPick[ip].t - pQuake[iq].t;

So it will be pass to bind code with wrong event.
Of course, this one will be killed with high probability.
But I think there is some possibility that event goes wrong. 

Change History

comment:1 Changed 4 weeks ago by paulf

  • Status changed from new to closed
  • Resolution set to fixed

Fixed in r7437.

comment:2 Changed 4 weeks ago by baker

I think the error is that the test for is_quake_simultaneous(i) is in the wrong place. It should be earlier than all the code that overwrites the next/last quake in the mQuake (=100) FIFO. That quake at the tail of the mQuake FIFO should be left alone until it is known that a newer lQuake replaces it. The way the code is written—and is only partially fixed by this patch—the tail of the mQuake FIFO is overwritten by the lQuake+1'st quake, which is then discarded if is_quake_simultaneous(i) without reinitializing (all zeros?) the entry. When that happens, that next/last entry in the mQuake FIFO is corrupt, since it no longer corresponds to the lQuake'th event. I don't know the code so I can't say that the mQuake FIFO ever has all mQuake valid events, which can lead to the aliasing Jin-Koo Lee describes. To be safe, the logic should only allow 99 valid events plus a spare for the next one.

Bottom line: this is not a complete fix of the actual error.

comment:3 Changed 3 weeks ago by baker

I think the code in grid_stack() in grid.c, just before it overwrites pQuake[i] (near line 870) should do the equivalent (if not actual) bind_kill() (in bind.c) to flush any picks associated with the previous lQuake that shares the same element in the pQuake[] FIFO. The problem is figuring out what that previous lQuake no. is to match up with the deleted pPick[ip].quake picks in bind_kill(). (I don't see the value of lQuake being saved in the pQuake[] element.) Unless this is done, when the code in grid_stack() overwrites pQuake[i], but then later decides not to increment lQuake, the picks that used to point to the tail pQuake[] FIFO entry are no longer pointing to their proper quake—it has been overwritten. But, since lQuake is not incremented, it looks like the old pQuake[] entry is still valid. Basically, the state of the pQuake[] FIFO changes, even though lQuake does not. The state of the FIFO (plural? I think there is a picks FIFO too?) is not preserved, and, I believe is inconsistent.

Note: See TracTickets for help on using tickets.