JOSS article

[AndreWeiner]

Dear @gerlero,

first of all, thank you for sharing your library. Loading OpenFOAM files in Python is often cumbersome. I have written OpenFOAM field parsers in the past, and I know how challenging it can be, especially when dealing with binary data. foamlib looks like an up-and-coming tool for workflow automation, data analysis, and ML applications.

At the time of writing, I haven't installed and tested foamlib yet, so this issue concerns only a couple of thoughts I had when reading the overview article (and briefly looking through the code). @akashdhruv, @paugier, @Failxxx maybe you want to add your remarks to this issue to avoid duplication.

1. line 20: could you comment on why you decided to write a parser rather than create C++ bindings? Maybe a short discussion of the pros and cons of different approaches could be helpful for readers.
2. line 66: How is parallelism handled when creating a FoamCaseBase instance? Let's say I simulated in parallel and then reconstructed the time folders. Will TimeDirectory loop over the reconstructed or the decomposed time folders?
3. line 122: How is the SLURM execution configured? I couldn't find any information in the documentation or the API.
4. line 129: I haven't tested the code yet, but I believe it is unlikely that the parser supports the full extent of OpenFOAM syntax. For example, have you considered regular expressions, macro expansions, or include directives (overview)? These expressions are frequently encountered in all sorts of dictionaries. Maybe you have to soften fully understands slightly. In general, I think it would be sensible to include a section with unsupported file format and other features in the README or the documentation and to refer to it in the article. For example, I also noticed that the current implementation for computing the cell centers will not work for distributed meshes, which is perfectly fine. Still, to avoid upsetting users, it's better to document it somewhere.
5. line 131-132: I guess FoamFile does not support outputs written to postProcessing like force coefficients, probes, slices, etc. I suggest clarifying this point since users/readers might not know that not all OpenFOAM output files are FoamFiles.
6. table 1: in the second column of uniform field, you write np.ndarray while in other lines, you use numpy.ndarray; if possible in terms of space, I suggest using either one or the other.

Don't hesitate to ask for clarification if any of my points are unclear.

Cheers, Andre

[gerlero]

Thanks, @AndreWeiner! Heads up, I’m on vacation right now; I’ll be replying next week.

[AndreWeiner]

All right, thanks for letting me know. Take your time and enjoy your vacation.
Cheers

[Failxxx]

Related to: openjournals/joss-reviews/issues/7633

[gerlero]

@AndreWeiner thanks again for the review! I'll reply to your comments one by one, listing the changes that I intend to make in the paper or docs (and then checking them off as I make them).

1. line 20: could you comment on why you decided to write a parser rather than create C++ bindings? Maybe a short discussion of the pros and cons of different approaches could be helpful for readers.

• PAPER: Comment on why an external parser was written instead of C++ bindings for OpenFOAM's own parser. (Revise article #324)

2. line 66: How is parallelism handled when creating a FoamCaseBase instance? Let's say I simulated in parallel and then reconstructed the time folders. Will TimeDirectory loop over the reconstructed or the decomposed time folders?

• PAPER: Mention that looping over a TimeDirectory will iterate over the reconstructed time folders. (Revise article #324)

3. line 122: How is the SLURM execution configured? I couldn't find any information in the documentation or the API.

• DOCS: describe how Slurm-based execution works with AsyncSlurmFoamCase. (Revise documentation #326)

4. line 129: I haven't tested the code yet, but I believe it is unlikely that the parser supports the full extent of OpenFOAM syntax. For example, have you considered regular expressions, macro expansions, or include directives (overview)? These expressions are frequently encountered in all sorts of dictionaries. Maybe you have to soften fully understands slightly. In general, I think it would be sensible to include a section with unsupported file format and other features in the README or the documentation and to refer to it in the article. For example, I also noticed that the current implementation for computing the cell centers will not work for distributed meshes, which is perfectly fine. Still, to avoid upsetting users, it's better to document it somewhere.

• DOCS: mention that the FoamFile class supports regular expressions, include directives, and other #-based expansions; but does not itself expand these expressions. (Revise documentation #326)
• DOCS: mention that FoamFile does not (currently) support #codeStreams. (Revise documentation #326)
• DOCS: mention that TimeDirectory cell center computation only works with reconstructed meshes. (Revise documentation #326)
• PAPER: soften language on statement of what files FoamFile can parse & refer readers to FoamFile API docs. (Revise article #324)

5. line 131-132: I guess FoamFile does not support outputs written to postProcessing like force coefficients, probes, slices, etc. I suggest clarifying this point since users/readers might not know that not all OpenFOAM output files are FoamFiles.

• DOCS: mention that non-FoamFile output files are also not supported by the FoamFile class. (Revise documentation #326)

11. table 1: in the second column of uniform field, you write np.ndarray while in other lines, you use numpy.ndarray; if possible in terms of space, I suggest using either one or the other.

• PAPER: fix inconsistency numpy.ndarray vs np.ndarray. (Revise article #324)

I'll start work on making these modifications. Do let me know anytime if you find that any of the proposed changes don't adequately respond to your remarks.