Conversation
…setting global vars)
Docmissing
rebuild checkpoint strategy
added mixing tests
fixed paths
add comments, docs, removed all TODOS
update readme with correct options
SamNewcome
left a comment
SamNewcome
left a comment
There was a problem hiding this comment.
Well done! The code is of a very high standard and I only really have some small comments of which the worst are really just that some variable/function names are a bit too vague.
| } | ||
| { | ||
| cells_[cell_index].clear(); | ||
| cells_[cell_index].shrink_to_fit(); |
There was a problem hiding this comment.
As far as I can see (ignore me if I'm mistaken), you only change the capacity of the cell's vector using shrink_to_fit (after a clear) and push_back. I suspect this is quite inefficient: You constantly will be reallocating memory to expand the size of these vectors. It is typically better to maintain a certain amount of unused but allocated memory if you suspect you will need it.
There was a problem hiding this comment.
in short: in theory maybe yes, in practice we did not really see a disadvantage.
Almost all of the time the halo cells are empty, so this won't do much. We profiled the whole function with the big simulation, this part of the code takes a negligible amount of time. To be honest, on our scale of the simulation, it does not really matter anyways, however, having the big picture in mind, we think it is always nice to free unneeded memory.
| * @param avg_velocity average velocity of particles | ||
| * @return the temperature according to the thermal motion | ||
| */ | ||
| double getThermalTemperature(ParticleContainer& particle_container, |
There was a problem hiding this comment.
All temperature is thermal. Of course this makes total sense to me or you, but to somebody else the distinction doesn't make any sense.
There was a problem hiding this comment.
A lot of the doxygen comments are "out-of-date" from the old interface Iterator and aren't appropriate here.
| particle_container.pairIterator([this](Particle& p, Particle& q) { | ||
| dvec3 f = {0, 0, 0}; | ||
| for (const auto& interactive_force : interactive_forces_) { | ||
| f = f + interactive_force->directionalForce(p, q); | ||
| } | ||
| p.addF(f); | ||
| q.subF(f); | ||
| }); |
There was a problem hiding this comment.
A little code duplication between here and the non-_OPENMP case. I actually don't care so much about the code duplication, but I would generally recommend to enclose as little code as possible in CMake macro branching, to improve code maintainability because mistakes with one branch may require recompiling (e.g. in the CI) to spot. So here I would simply prefer to somewhere force the strategy to 3 if OpenMP is disabled and have this part of the code outside of CMake branching (you can always add warnings if the strategy is not 3 but OpenMP is disabled)
| #ifndef MEMBRANEGENERATOR_H | ||
| #define MEMBRANEGENERATOR_H |
There was a problem hiding this comment.
You don't need this is you have #pragma once
| /** | ||
| * @brief number of particles, that are immovable | ||
| */ | ||
| size_t special_particle_count_{}; |
There was a problem hiding this comment.
"Special particle" doesn't describe what is different about these particles. Similarly, one of the getter docs doesn't explain this in their doxygen. Only the brief for the actual variable and for one of the getter docs explains what is special about these particles. Be clear with your variable names e.g. immovable_particle_count. (By two getters docs, I mean that you have two doxygen documentations for the same function)
| /** | ||
| * @brief the exact number of current particles, updated accordingly | ||
| * @return the current count of particles left in the simulation | ||
| */ |
There was a problem hiding this comment.
The documentation should distinguish between this and the non-const version of this function.
There was a problem hiding this comment.
I also think it's neater to pair const and non-const getters immediately next to each other - it wasn't immediately obvious what the difference was.
| std::cout << (*it)->getX()[0] << ", " << (*it)->getX()[1] << ", " | ||
| << (*it)->getX()[2] << std::endl; |
There was a problem hiding this comment.
You can be clearer with this: make it an error log or throw an exception. If the cell is not valid then pushing the particle pointer is likely to cause a segmentation fault - a cleaner exception is nicer.
| EXPECT_EQ(container.getParticleCount(), 4) << "Particle Count wrong"; | ||
|
|
||
| statistics.writeStatistics(1); | ||
| statistics.closeFiles(); | ||
|
|
||
| std::ifstream dfile(densityFile); | ||
| std::ifstream vfile(velocityFile); | ||
|
|
||
| if (!dfile || !vfile) { | ||
| FAIL() << "Could not open at least one of the files"; | ||
| } | ||
|
|
||
| std::string line; | ||
| std::getline(dfile, line); | ||
| ASSERT_EQ(line, "1,0.222222,0.000000,0.111111"); | ||
|
|
||
| std::getline(vfile, line); | ||
| ASSERT_EQ(line, "1,2.500000 2.500000 1.500000,0.0 0.0 0.0,4.000000 3.000000 3.000000"); |
There was a problem hiding this comment.
EXPECT_EQ and ASSERT_EQ would be better the other way around
| /** | ||
| * note by the author: this does test ALL combinations for a 3x3 (and therefore | ||
| * 5x5) cube. I spent too much time doing this | ||
| */ |
There was a problem hiding this comment.
It's super nice that you have this. I know this is the end of the course, but in real codes such detailed tests are really worthwhile.
You should consider parameterised tests (or simply using helper functions) to help reduce code duplication. This makes it easier to fix bugs which can easily occur when writing so many tests like this by hand or with gen AI.
5 participants
No description provided.