Feature: OpenFOAM reader [1/n] - First structure#2256
Feature: OpenFOAM reader [1/n] - First structure#2256sandro-elsweijer wants to merge 7 commits intomainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2256 +/- ##
=======================================
Coverage 78.50% 78.50%
=======================================
Files 115 115
Lines 19198 19198
=======================================
Hits 15071 15071
Misses 4127 4127 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
spenke91
left a comment
There was a problem hiding this comment.
Thanks a lot for this feature @sandro-elsweijer ! I really like the code structure and style 👍 I do have some remarks, though, most of them being minor issues...
I went through the code, but did not run the example myself yet, since I did not have an OpenFOAM case readily available and would argue that we should add an example case anyway 😇
| using t8_path = std::filesystem::path; | ||
|
|
||
| /** | ||
| * Reader for OpenFOAM cases. |
There was a problem hiding this comment.
| * Reader for OpenFOAM cases. | |
| * Constructor of OpenFOAM reader. |
| error = !read_points (case_points_dir); | ||
| error &= !read_faces (case_faces_dir); | ||
| error &= !read_owner (case_owner_dir); | ||
| error &= !read_neighbor (case_neighbor_dir); |
There was a problem hiding this comment.
As far as I know, &= is for bitwise AND, not logical AND. While it does in fact work for bools, I think we should reseve it for bitwise operations for code readability. I thus suggest:
| error = !read_points (case_points_dir); | |
| error &= !read_faces (case_faces_dir); | |
| error &= !read_owner (case_owner_dir); | |
| error &= !read_neighbor (case_neighbor_dir); | |
| error = !read_points (case_points_dir); | |
| error = error && !read_faces (case_faces_dir); | |
| error = error && !read_owner (case_owner_dir); | |
| error = error && !read_neighbor (case_neighbor_dir); |
or with and instead of &&, which, personally, I like even more ;-)
| @@ -0,0 +1,490 @@ | |||
| /* | |||
There was a problem hiding this comment.
What is your motivation behind having the full implementation in the header file instead of a source file? None of the standard cases (template, inline, ...) seems to apply, or did I miss something? 🤔
| std::string line; | ||
| bool format_checked = false; | ||
|
|
||
| /* Loop through file till end of file or thill we break manually at the right position. */ |
There was a problem hiding this comment.
| /* Loop through file till end of file or thill we break manually at the right position. */ | |
| /* Loop through file till end of file or till we break manually at the right position. */ |
| size_t numeric_value; | ||
| if (line_stream >> numeric_value) { | ||
| /* Numeric value found, move stream to start of line again */ | ||
| input_stream.seekg (-static_cast<std::streamoff> (line.size ()) - 1, std::ios_base::cur); |
There was a problem hiding this comment.
Why do we have to move input_stream back to the start of the line? Isn't it still at the start of the line anyway? Because the only command in the while loop modifying it is in the condition, i.e., std::getline (input_stream, line)). Or did I miss something?
| } | ||
|
|
||
| /* The number of cells in the mesh is the highest owner_id + 1 (since it starts at 0). */ | ||
| const size_t num_cells = *std::max_element (owner_list.begin (), owner_list.end ()) + 1; |
There was a problem hiding this comment.
I guess this part is not expected to be very performance-critical. Otherwise it might make sense to keep track of the maximum already in the for loop above filling the vector. This way we would not have to iterate through the whole vector again. But it would just be a memory access / caching thing, not so much a complexity issue, so I guess it is not important at all :D
| } | ||
|
|
||
| /** | ||
| * Reads in an OpenFOAM owner file, assigns the faces to their cells and |
There was a problem hiding this comment.
| * Reads in an OpenFOAM owner file, assigns the faces to their cells and | |
| * Reads in an OpenFOAM neighbor file, assigns the faces to their cells and |
Right?
| /** | ||
| * Reads in an OpenFOAM owner file, assigns the faces to their cells and | ||
| * also fills the other half of the neighborhoods. | ||
| * \param [in] neighbor_dir The path to the points file. |
There was a problem hiding this comment.
As above, maybe neighbor_file?
| build_cmesh () | ||
| { | ||
| t8_global_errorf ("ERROR: Not implemented yet. \n"); | ||
| return 0; |
There was a problem hiding this comment.
| return 0; | |
| return false; |
| @@ -0,0 +1,120 @@ | |||
| /* | |||
There was a problem hiding this comment.
Could you add an example case file? I think it would really improve this example if the user could test it for an example file easily without having to find one him-/herself 😬
Closes #2257
Describe your changes here:
Implements a first structure of the OpenFOAM reader.
The first draft of the OpenFOAM reader struct reads the files inside an OpenFOAM case and fills internal data structures.
The conversion of this data into a cmesh follows in the next PRs. Otherwise this PR would be too big to review.
The reader is not inside cmesh/IO/OpenFOAM, since the reader will not only provide a cmesh, but also a forest with data. So there is no clear distinction between the reader cmesh and forest and the OpenFOAM functionality is its own category.
This PR also implements an OpenFOAM example and it has two purposes:
There is no test yet, since the OpenFOAM reader is not finished yet. I plan to implement two tests:
Both tests can only be written, when the reader is finished.
All these boxes must be checked by the AUTHOR before requesting review:
Documentation:,Bugfix:,Feature:,Improvement:orOther:.All these boxes must be checked by the REVIEWERS before merging the pull request:
As a reviewer please read through all the code lines and make sure that the code is fully understood, bug free, well-documented and well-structured.
General
Tests
If the Pull request introduces code that is not covered by the github action (for example coupling with a new library):
Scripts and Wiki
scripts/internal/find_all_source_files.shto check the indentation of these files.License
doc/(or already has one).