FireDynamics / fdsreader

Python reader for FDS data
GNU General Public License v3.0
44 stars 18 forks source link

Bugfix for 3D Slices #48

Closed r-wrobel closed 1 year ago

r-wrobel commented 1 year ago

I wasn't able to open 3D-Slices. This changes solved the problem.

get_2d_slice_from_3d:

to_global:

JanVogelsang commented 1 year ago

Thanks for looking into this, very much appreciated! I will actually not merge this pull request, but instead incorporate the changes myself, as I would like to restructure the code a bit while doing so.
All your suggested changes in the get_2d_slice_from_3d-method are correct though, thanks for that!

Regarding the to_global-method, I don't yet see how changing
slc_data = slc.data if slc.orientation == 0 else np.expand_dims(slc.data, axis=slc.orientation)
to
slc_data = slc.data
would still work for 2D slices. And I am also not sure what the additional lines for the mask are supposed to do. Could you explain what you did there?

r-wrobel commented 1 year ago

Glad you looked at it. I made a couple of fast changes for one special case with only 3D slices and didn't mind about 2D.

I'm not very familiar with the code. Does a difference between a 2D slice and a get_2d_slice_from_3d slice exist? I assumed slc.orientation the direction x|y|z resp. 1|2|3 and didn't find the reason for adding an additional dimension to the slice. All i can say is, that it only worked for me without np.expand_dims

Regarding mask: For non cell_centered slices slc_data is changed in line 554. therefore the mask was to big. If slc_data is unchanged, the added lines shouldn't do anything. Due to performance, there could be an if-condition filtering cell_centered slices, so you skip the for-loop, which would change nothing.

JanVogelsang commented 1 year ago

No difference exists between slices cut from a 3D one and regular 2D ones, but the to_global-method is supposed to work for 3D slices out of the box as well.

Regarding the mask, I will look into this, not too sure yet if this isn't actually a bug somewhere else as well.

I will try out the to_global-method for 3D slices soon. Could you send me an FDS-case with 3D slices that actually makes sense to test the method on a real example instead of a constructed one.

r-wrobel commented 1 year ago

Testcase-Tank.fds.txt is a simplified version of the file because of which I needed the changes to the FDSreader.

JanVogelsang commented 1 year ago

I incorporated your changes into the code and make it work for both 2D and 3D slices now. I will therefore close this PR now.