Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limited functionality for simulations ran with nightly build FDS version #72

Open
ManuelOsburg opened this issue Mar 12, 2024 · 11 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@ManuelOsburg
Copy link

I have identical two FDS simulations. One was run with FDS-6.8.0-release and another with FDS-6.8.0-1386-g42fde33-nightly. While there are no problems with the simulation calculated with FDS-release, the fdsreader functionality is limited with the simulation calculated with FDS-nightly. In particular, the BNDF files cannot be read.

FDS-release
image

FDS-nightly:
image

@ManuelOsburg ManuelOsburg changed the title Limited functionality for simulations ran with nightly build version Limited functionality for simulations ran with nightly build FDS version Mar 12, 2024
@JanVogelsang JanVogelsang self-assigned this Mar 13, 2024
@JanVogelsang JanVogelsang added the wontfix This will not be worked on label Mar 13, 2024
@JanVogelsang
Copy link
Collaborator

Yes, that might very well be the case, as the nightly build incorporates wild changes of the FDS development team that might very well never make it to an actual release. As the FDS team is not supporting this project in any way, it's impossible to keep up with their daily changes. It's even difficult to make the FDSreader compatible with various FDS release versions, as the FDS team is not managing to make their versions backward-compatible.
So unfortunately, we can't offer any support for nightly builds. They might work, but might also not work. You can create an issue at FDS thought and indicate that their nightly builds are not producing the same (binary) outputs as their release versions, but I would assume they are aware of that and simply don't guarantee any consistency between versions.

@JanVogelsang JanVogelsang closed this as not planned Won't fix, can't repro, duplicate, stale Mar 13, 2024
@amarcozzi
Copy link

Hi, @JanVogelsang. Now that these changes have transitioned from the nightly release to the new minor release version 6.9 of FDS, would you be open to integrating this change? I have a member of my team that can work on a fix for the new output file format.

@JanVogelsang
Copy link
Collaborator

@amarcozzi Sure thing, such a fix would be very welcome! He could either create a pull request directly or just describe how the file format has changed, so that I can integrate the changes into the code!

@JanVogelsang JanVogelsang reopened this Mar 25, 2024
@JanVogelsang JanVogelsang added enhancement New feature or request help wanted Extra attention is needed and removed wontfix This will not be worked on labels Mar 25, 2024
@amarcozzi
Copy link

Thanks, @JanVogelsang. My teammate and I talked about this yesterday and she's going to start looking into a fix. This is related to issue #73 as well just so you're aware.

@briannak7
Copy link

@JanVogelsang, the FDS developers informed me that the .bnd files have been changed and no longer output the minimum and maximum values for each timestep.

When looking at the source code for fdsreader, it appears that Simulation._load_boundary_data is reading the number of timesteps (n_t) from these .bnd files. Consequently, when the Patch object is created in this method, n_t is set to 1 due to the file changes. This is why, I think, we’re only seeing the one timestep of boundary data.

Since these files are now differently structured, I wondered if reading in the data from the .bf files rather than the .bnd files would be a good approach. If so, any ideas on how to start with this change would be appreciated!

@JanVogelsang
Copy link
Collaborator

@briannak7 Are they now outputting the number of time steps somewhere in .bf files instead? If not it's quite difficult to read in the data, as you can't just read in binary data without knowing how you should actually interpret the data. It might be possible to calculate the number of time steps using the file size though, I think I have done that for some other data type as well.

@briannak7
Copy link

@JanVogelsang, the .bf files don't contain the number of time steps directly, but we can grab the lower and upper bounds at each timestep within the .bf file and set n_t = len(times). It looks like we're already getting data from this file in _load_boundary_data and this could be a simple addition.

I wasn't sure if the boundary data is lazy loaded or not. If not, reading in the data this way could be time consuming and getting the number of timesteps from the file size may be optimal upon initializing the simulation.

I have working code to read in the .bf file, and I'm currently working on incorporating it into Simulation._load_boundary_data. I hope to have this finished later this week. Let me know if you have any thoughts or feedback!

@JanVogelsang
Copy link
Collaborator

@briannak7 So the times or lower/upper bounds are now stored in the .bf-file? Or how would we get "len(times)"?

Yes, the patch data (boundary data for one side of an obstruction of one mesh) is lazy loaded, but the .bf-file is still read to load some meta-data, but only the file header is actually read. Looking forward to seeing your implementation!

@briannak7
Copy link

@JanVogelsang the times, lower, and upper bounds are stored in the .bf file which is how we can get len(times).

Thanks for letting me know this is lazy loaded!

@JanVogelsang
Copy link
Collaborator

But how do you load the times without knowing how many times to load? Sounds like a chicken and egg problem.

@amarcozzi
Copy link

Hi @JanVogelsang sorry for the delay on this. I implemented a quick fix and created PR #79 so that you could see the change. I intend to update PR #79 with feedback from this issue discussion.

To refresh our memory about the problem, the .bnd files containing the time, lower, and upper bounds and timestep are deprecated as of FDS 6.9. Rather than reading that data from the .bnd text files, I propose that we read the data directly from the binary files.

PR #79 implements this for the boundary data by reading the headers, patch information, and computing n_t based on the number of bytes in the file. Then, it reads in the time for each timestep by updating the file offset. This approach no longer reads in the lower and upper bounds data because it is quite computationally expensive to allocate an array for each patch, compute the min/max, then do that for each timestep. It would also in effect read in the data twice. So I chose not to include those values.

Let me know what you think of this approach.

@JanVogelsang JanVogelsang removed their assignment Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants