FireDynamics / fdsreader

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

Slice coordinates #47

Closed lu-kas closed 1 year ago

lu-kas commented 1 year ago

Hi!

I think this difference needs to be an absolute one: https://github.com/FireDynamics/fdsreader/blob/7a727bd6b7dbf541f7045ca76f961c3826f2986d/fdsreader/slcf/slice.py#L343

I.e. if np.abs(coords[dim][i] - coords[dim][i+1]) < min_delta/2:

Additionally, a numpy Array does not have a remove function, afaik. https://github.com/FireDynamics/fdsreader/blob/7a727bd6b7dbf541f7045ca76f961c3826f2986d/fdsreader/slcf/slice.py#L346

Finally, shouldn't a Slice object compute the coordinates to match the data set created with to_global? This could be done by using the extent and the number of grid points in the according directions. What do you think?

JanVogelsang commented 1 year ago

I think this difference needs to be an absolute one:

Either an absolute one or simply change the order of the difference (as coords[dim] is sorted).

Additionally, a numpy Array does not have a remove function, afaik.

You are right, I probably added the np.array(...) later without thinking about the consequences. I need to change that.

Finally, shouldn't a Slice object compute the coordinates to match the data set created with to_global? This could be done by using the extent and the number of grid points in the according directions. What do you think?

I think the to_global method should probably return its own coordinates array (maybe make it optional), as it should also be possible to just get all the coordinates at which values of the slice exist. I can't think of a particular use case yet, but why not simply offer both options.

lu-kas commented 1 year ago

I think this difference needs to be an absolute one:

Either an absolute one or simply change the order of the difference (as coords[dim] is sorted).

True. If possible, could you please create a quick fix and release a new version? Thanks! (We have a workshop tomorrow, where I would like to use this function...)

Additionally, a numpy Array does not have a remove function, afaik.

You are right, I probably added the np.array(...) later without thinking about the consequences. I need to change that.

The question is why would you want to remove any values at all. If you have coordinates 'close' to each other, then the meshes are not aligned or have different resolutions, thus a global coordinate system may not be reasonable. Having a coordinate system on the sub-slice level is useful.

Finally, shouldn't a Slice object compute the coordinates to match the data set created with to_global? This could be done by using the extent and the number of grid points in the according directions. What do you think?

I think the to_global method should probably return its own coordinates array (maybe make it optional), as it should also be possible to just get all the coordinates at which values of the slice exist. I can't think of a particular use case yet, but why not simply offer both options.

Yes, this is a very good proposal.

JanVogelsang commented 1 year ago

Will do!

Didn't think of the case that meshes could potentially not be aligned perfectly, should be a really rare case. The reason I remove values here is because of floating point problems, which is an even bigger problem when reading in floats from files where they were written with limited precision. I often encountered cases, where I had very similar coordinates (e.g. 1.0 and 1.000001). So I implemented a dynamic threshold to detect those cases, where the treshold was simply half of the minimum step size, which should ensure that even if somebody was to use meshes with a very low resolution, I would still detect floating point issues in a nice way. Your point is valid though, and I should probably use a fixed threshold instead to consider two float values as equal. I think 20^-6 will be a good absolute threshold.

JanVogelsang commented 1 year ago

Fixed in c8b9819 (version 1.9.1)

lu-kas commented 1 year ago

Thank you!

lu-kas commented 1 year ago

Is this output needed or just an artifact?

https://github.com/FireDynamics/fdsreader/blob/c8b9819d685fbb004f304d4700c11fbf5ff31fd1/fdsreader/slcf/slice.py#L343

JanVogelsang commented 1 year ago

Damn, that's an artifact.