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

JOSS article #316

Open
AndreWeiner opened this issue Jan 27, 2025 · 3 comments
Open

JOSS article #316

AndreWeiner opened this issue Jan 27, 2025 · 3 comments

Comments

@AndreWeiner
Copy link

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
Copy link
Owner

gerlero commented Jan 27, 2025

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

@AndreWeiner
Copy link
Author

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

@Failxxx
Copy link

Failxxx commented Feb 3, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants