Conversation
…file, config integration pending @tim
# Conflicts: # src/lib/simulator/physics/Thermostat.h
fix: XML file adaptation
thesamriel
left a comment
thesamriel
left a comment
There was a problem hiding this comment.
Note
This is only feedback regarding the code directly. Grade feedback will be on Moodle once we are through with all groups.
Thanks a lot for this strong submission. I think your code improved a lot in terms of readability and general structure while still using modern features. I still think it's great that you experimented with even more things from C++23, but I guess you learned that it can also be quite a hassle to get things running with the newest versions available. (unfortunately :( )
Your parallelization strategies especially are really well implemented.
It seems like you ran out of time for the membrane simulation in the end, which is a bit sad. But overall, I think you have a really nice repository and I hope you also learned a lot :)
| # create make target | ||
| add_executable(MolSim src/bin/MolSim.cpp) | ||
| add_executable(MolSim src/bin/MolSim.cpp | ||
| src/lib/simulator/physics/ProfileCalculator.h) |
There was a problem hiding this comment.
Not sure why you needed to add this file explicitly here
| break; | ||
| } | ||
| default: | ||
| SPDLOG_ERROR("Unsupported Force with Linked Cells"); |
There was a problem hiding this comment.
| SPDLOG_ERROR("Unsupported Force with Linked Cells"); | |
| SPDLOG_ERROR("Unsupported Force with Vector container"); |
| auto simulator = setup<2>(config); | ||
| startTime = std::chrono::high_resolution_clock::now(); | ||
| if (config->output_frequency == 0) { | ||
| simulator.run<false>(); | ||
| } else { | ||
| simulator.run<true>(); | ||
| } | ||
| } else if (config->dimensions == 3) { | ||
|
|
||
| auto simulator = setup<3>(config); | ||
| startTime = std::chrono::high_resolution_clock::now(); | ||
| if (config->output_frequency == 0) { | ||
| simulator.run<false>(); | ||
| } else { | ||
| simulator.run<true>(); | ||
| } |
There was a problem hiding this comment.
A nice way to reduce code duplication in cases like here can be lambda expressions.
| auto simulator = setup<2>(config); | |
| startTime = std::chrono::high_resolution_clock::now(); | |
| if (config->output_frequency == 0) { | |
| simulator.run<false>(); | |
| } else { | |
| simulator.run<true>(); | |
| } | |
| } else if (config->dimensions == 3) { | |
| auto simulator = setup<3>(config); | |
| startTime = std::chrono::high_resolution_clock::now(); | |
| if (config->output_frequency == 0) { | |
| simulator.run<false>(); | |
| } else { | |
| simulator.run<true>(); | |
| } | |
| auto runSimulation = [&](auto &simulator) { | |
| if (config->output_frequency == 0) { | |
| simulator.run<false>(); | |
| } else { | |
| simulator.run<true>(); | |
| } | |
| }; | |
| auto simulator = setup<2>(config); | |
| startTime = std::chrono::high_resolution_clock::now(); | |
| runSimulation(simulator); | |
| } else if (config->dimensions == 3) { | |
| auto simulator = setup<3>(config); | |
| startTime = std::chrono::high_resolution_clock::now(); | |
| runSimulation(simulator); |
| uint8_t fixed{}; | ||
|
|
||
| uint8_t is_membrane{}; |
There was a problem hiding this comment.
These variable names assume booleans. I guess you made them uint8_t for memory alignment in the SoAs? In production, I would first benchmark the difference and a then add a comment why these need to be integers.
There was a problem hiding this comment.
Git often does not recognize file renames. To have a cleaner git history, I would use git mv old_filename new_filename when renaming or moving files within a git repository.
| p.positions[i][index] += delta_t * p.velocities[i][index] + temp * p.old_forces[i][index]; | ||
| if (p.fixed[index] == 0) [[likely]] { | ||
| for (size_t i = 0; i < DIMENSIONS; ++i) { | ||
| p.positions[i][index] += delta_t * p.velocities[i][index] + temp * p.old_forces[i][index]; |
There was a problem hiding this comment.
We updated the slides for next semester since you are not the only group with this mistake.
But you should use the updated forces here, not the old ones.
| const auto diff = (std::array<double, 3>({x2, y2, z2}) + correction) - std::array<double, 3>({x1, y1, z1}); | ||
| const auto norm = ArrayUtils::L2Norm(diff); | ||
|
|
||
| std::array<double, 3> result = {0, 0}; |
There was a problem hiding this comment.
| std::array<double, 3> result = {0, 0}; | |
| std::array<double, 3> result{}; |
Empty curly braces zero-initialize. Then you avoid this awkward 2 dim initialization for a vector of 3.
| auto average_velocity = calculateAverageVelocity(particles); | ||
| adjustVelocities(particles, average_velocity); |
There was a problem hiding this comment.
I think you are including the wall particles here which leads to skewed results. The wall particles should not be considered as 'actual particles' but rather our way to model a wall
4 participants
No description provided.