firemodels / fds

Fire Dynamics Simulator
https://pages.nist.gov/fds-smv/
Other
664 stars 624 forks source link

Unexpected behaviour of angled beam detectors #4684

Closed bmralph closed 7 years ago

bmralph commented 7 years ago

Hi guys. Been working with beam detectors a lot recently - specifically angled ones. @gforney kindly implemented a method to visualise beams in smv. Doing this I noticed something odd - when beams were angled with either X2, Y2or Z2being less than X1, Y1, Z1respectively the beams were displayed incorrectly. The beams would always span from the lowest XYZ values to the highest. e.g. a beam with XB=0,1,0,1,1,0 would be shown as a beam with XB=0,1,0,1,0,1.

I didn't know whether this was a problem with just the visualisation so I ran some test cases beam_example.fds.txt and found that this isn't a visualisation problem but the beam actually does span from lowest points to greatest points - irrespective of the XB definition.

The test case has a INIT volume of soot that only one of the beams (low2highbeam) should record - but both receive it. high2lowbeam should miss the volume of soot.

I have been trying to debug and I see that pairs of XB(1) and (2), (3) and (4), (5) and (6) swap to put the lowest value in (1), (3) and (5). This is what is causing the problems with the beams. There will also be some carry on impact on the code I think regarding how cells are applied to the I/J/K_PATH matrices for regressing cell numbers. I can't sort the problem within the PATH OBSCURATION CASE in read.f90 as by that time the incorrect/swapped/sorted values are present.

Any thoughts?

rmcdermo commented 7 years ago

Thanks, Ben. Someone will look at this shortly.

mcgratta commented 7 years ago

If we remove the CALL CHECK_XB from the subroutine READ_DEVC in read.f90

   IF (POINTS==1 .AND. XB(1)>-1.E5_EB) THEN

      CALL CHECK_XB(XB)   <--- Kill this?

      BAD = .TRUE.

is the problem solved?

bmralph commented 7 years ago

I'm away this weekend and can't check this out until Monday evening. I'll have a look then.

On Fri, 27 Jan 2017 at 21:18, Kevin McGrattan notifications@github.com wrote:

If we remove the CALL CHECK_XB from the subroutine READ_DEVC in read.f90

IF (POINTS==1 .AND. XB(1)>-1.E5_EB) THEN

  CALL CHECK_XB(XB)   <--- Kill this?

  BAD = .TRUE.

is the problem solved?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/firemodels/fds/issues/4684#issuecomment-275776977, or mute the thread https://github.com/notifications/unsubscribe-auth/AMm1wWJe-xdOaB7OtCB0Vk899FtuVExUks5rWl8HgaJpZM4Lv7m7 .

drjfloyd commented 7 years ago

That should solve the problem. The PATH OBSCURATION integrates the soot density from XB(1),XB(3),XB(5) to XB(2),XB(4),XB(6) so if check XB switches say XB(1) and XB(2) then it wouldn't be the path the user gave. Our beam detector verification case has an angled device, but it is in a uniformly smoke filled box so this error wouldn't have gotten trapped. Would this have unintended effects? Line devices and PATH OBSCURATION we clearly want to preserve the input XB, but if we avoid the check would that break volume or area integral devices?

bmralph commented 7 years ago

But shouldn't the user be inputting e.g. X(1) < X(2) anyway to correctly bound the volume? Only skipping this check for PATH OBSCURATION would be an option too?

mcgratta commented 7 years ago

I committed a change so that the coordinates will not be changed for PATH OBSCURATION or TRANSMISSION (#4688). I thought of creating a descriptor for OUTPUT_QUANTITY to allow us to label quantities as either path or volume specific, but we do not process quantity info until after we process coordinate info. So for now, we just explicitly restrict certain quantities.

Test this and close the issue if things are OK.

bmralph commented 7 years ago

Thanks! I'll check tomorrow :)

On 30 Jan 2017 18:43, "Kevin McGrattan" notifications@github.com wrote:

I committed a change so that the coordinates will not be changed for PATH OBSCURATION or TRANSMISSION (#4688 https://github.com/firemodels/fds/pull/4688). I thought of creating a descriptor for OUTPUT_QUANTITY to allow us to label quantities as either path or volume specific, but we do not process quantity info until after we process coordinate info. So for now, we just explicitly restrict certain quantities.

Test this and close the issue if things are OK.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/firemodels/fds/issues/4684#issuecomment-276151522, or mute the thread https://github.com/notifications/unsubscribe-auth/AMm1wXCSxyKAFgo03IxaxhQ2Z6_n_SLQks5rXi8-gaJpZM4Lv7m7 .

bmralph commented 7 years ago

@mcgratta this fixes the swapping of XB data and the beams are now located in the correct space from an integration of obscuration point of view. Two problems remain though: (1) The integration is incorrect (coded assuming X(1)<X(2). I will start looking at this today. (2) @gforney the beams are still shown angled the wrong way in SmokeView. i.e.

&DEVC XB=0,1,0,1,0,1,ID='low2highbeam',QUANTITY='PATH OBSCURATION'/
&DEVC XB=0,1,0,1,1,0,ID='high2lowbeam',QUANTITY='PATH OBSCURATION'/

are shown overlapping one another. Odd as the XB data isn't sorted any longer.

mcgratta commented 7 years ago

I'll fix (1).

mcgratta commented 7 years ago

I am looking in read.f90 where PATH OBSCURATION is set up. Where do you see the requirement that X(2)>X(1)?

bmralph commented 7 years ago

There is no requirement coded. I'm just assuming that the integration is based on moving forward through increasing cell numbers.

One problem I have found is where DV%I_PATH etc. is populated in line 11442 onwards - there should be an IF statement X(2)>X(1) which will drop the +1. e.g.

DV%I_PATH = INT(GINV(DV%X1-M%XS,1,NM)*M%RDXI)   + 1

becomes

IF (DV%X2 < DV%X1) THEN
   DV%I_PATH = INT(GINV(DV%X1-M%XS,1,NM)*M%RDXI)
ELSE
   DV%I_PATH = INT(GINV(DV%X1-M%XS,1,NM)*M%RDXI) + 1
ENDIF

This correctly populates the I/J/K_PATH matrices and obviates an extra cell being added.

gforney commented 7 years ago

I added a beam &DEVC to ben's test case (see attached) to make sure smokeview was not messing up . My intent was that the beam detector go from (1,0,0) to (0,1,1) . when I run the lastest fds (on both windows and linux) I get the following crash

Mesh 1 is assigned to MPI Process 0 forrtl: severe (408): fort: (2): Subscript #1 of the array X has value 11 which is greater than the upper bound of 10

Image PC Routine Line Source
fds_db 0000000002ABCAA6 Unknown Unknown Unknown fds_db 0000000001EF09E3 read_input_mp_pro 11465 read.f90 fds_db 0000000001CB751E read_input_mp_rea 136 read.f90 fds_db 0000000002637BE6 MAIN__ 104 main.f90 fds_db 000000000040989E Unknown Unknown Unknown libc.so.6 0000003F0581ED1D Unknown Unknown Unknown fds_db 00000000004097A9 Unknown Unknown Unknown

test.fds.txt

mcgratta commented 7 years ago

This is a problem with being on the border of two cells and choosing the upper value, which puts us out of the array. I'll fix.

mcgratta commented 7 years ago

I committed a fix to the PATH OBSCURATION calculation. Try your case again and let me know if things are working, backwards and forwards. I used the verification case Detectors/beam_detector.fds in which I reversed the direction of the paths.

gforney commented 7 years ago

my case is working now. thanks.

bmralph commented 7 years ago

My cases are now working correctly. I'll leave this open for a day or so - whilst I ensure that all my other cases work as expected. Thanks guys.

mcgratta commented 7 years ago

Can you post a simple version of a case that is not working.

bmralph commented 7 years ago

If I find one I will - the cases I've checked are working now.

bmralph commented 7 years ago

Appears to be working correctly now. Thanks :)