Assignment4 by laert-ll · Pull Request #34 · laert-ll/MolSim

laert-ll · 2024-06-21T00:39:23Z

No description provided.

…ticleGeneratorTest, FileReaderTest

# Conflicts: # src/Main.cpp # src/MolSim.h

…in but without the periodic tests to check if all the tests except the new ones pass

manishmishra6016

Note
This is only feedback regarding the code directly. Grade feedback will be on Moodle once we are through with all groups.

Thank you for your nice submission 💪
Unfortunately you could not run your big cases, probably because you had to spend time in implementing features from previous worksheet. But I am sure you will do a great job in final worksheet and you are on the right track :) Having said, if you feel like you have too much to do from previous worksheet, let us know via email and we can provide a working code that you can use. Ofc, if you want to fix and work with your own code, that is much appreciated 😄

I have left some comments, so go over them and see if they make sense. I tried to share some ideas about improving performance and code structure. Feel free to ask if something is not clear. Focus on finishing the tasks with priority, minor changes can be done later. Good luck with the worksheet :)

manishmishra6016 · 2024-06-25T11:30:21Z

+    std::unique_ptr<fileReaders::FileReader> fileReader;
+    std::unique_ptr<outputWriters::FileWriter> outputWriter;
+    std::shared_ptr<calculators::Calculator> calculator;
+    std::unique_ptr<Thermostat> thermostat = std::make_unique<Thermostat>(10, 20, 5, 1, 3);


I would recommend passing variables with meaningful names as arguments. If you are worried about performance, you can just declare them as const and it should be fine. But this is not a major issue, just a recommendation.

manishmishra6016 · 2024-06-25T11:35:20Z

+        /**
+        if (result.count("boundaries")) {
+            std::string boundariesArg = result["boundaries"].as<std::string>();
+            if (boundariesArg.size() == 4) {
+                std::array<boundaries::BoundaryDirection, 4> directions = {
+                        boundaries::BoundaryDirection::LEFT,
+                        boundaries::BoundaryDirection::BOTTOM,
+                        boundaries::BoundaryDirection::RIGHT,
+                        boundaries::BoundaryDirection::TOP
+                };

+                for (int i = 0; i < 4; ++i) {
+                    switch (boundariesArg[i]) {
+                        case '0':
+                            boundaryMap[directions[i]] = boundaries::BoundaryType::OFF;
+                            break;
+                        case '1':
+                            boundaryMap[directions[i]] = boundaries::BoundaryType::REFLECTING;
+                            break;
+                        case '2':
+                            boundaryMap[directions[i]] = boundaries::BoundaryType::OUTFLOW;
+                            break;
+                        case '3':
+                            boundaryMap[directions[i]] = boundaries::BoundaryType::PERIODIC;
+                            break;
+                        default:
+                            SPDLOG_WARN("Invalid boundary value '{}', defaulting all to PERIODIC", boundariesArg[i]);
+                            boundaryMap[directions[i]] = boundaries::BoundaryType::PERIODIC;
+                    }
+                }
+            } else {
+                SPDLOG_WARN("Invalid boundary string length, defaulting all boundaries to REFLECTING");
+                boundaryMap[boundaries::BoundaryDirection::LEFT] = boundaries::BoundaryType::REFLECTING;
+                boundaryMap[boundaries::BoundaryDirection::BOTTOM] = boundaries::BoundaryType::REFLECTING;
+                boundaryMap[boundaries::BoundaryDirection::RIGHT] = boundaries::BoundaryType::REFLECTING;
+                boundaryMap[boundaries::BoundaryDirection::TOP] = boundaries::BoundaryType::REFLECTING;
+            }
+        } else {
+            SPDLOG_WARN("No boundary conditions specified, defaulting all boundaries to REFLECTING");
+            boundaryMap[boundaries::BoundaryDirection::LEFT] = boundaries::BoundaryType::REFLECTING;
+            boundaryMap[boundaries::BoundaryDirection::BOTTOM] = boundaries::BoundaryType::REFLECTING;
+            boundaryMap[boundaries::BoundaryDirection::RIGHT] = boundaries::BoundaryType::REFLECTING;
+            boundaryMap[boundaries::BoundaryDirection::TOP] = boundaries::BoundaryType::REFLECTING;
+        }
+         **/


dead code - if you don't plan to use, just remove it. In general, do not keep dead code on master.

manishmishra6016 · 2024-06-25T11:41:16Z

-            if (iteration % 10 == 0) {
-                outputWriter->plotParticles(iteration, particleContainer, filename);
-            }
+        if (linkedCellsUsed) {


While it is fine for the worksheet, I would generally not use if-else here. You can think of Runtime Polymorphism to implement this. The main idea being that you have a pointer to a base class from which directSum and LinkedCell containers are derived. You can initialize the pointer to base class with object of the derived class during runtime based on the user input.

manishmishra6016 · 2024-06-25T11:52:18Z

+        REFLECTING, /**< Boundary reflects particles. */
+        OUTFLOW,    /**< Boundary allows particles to flow out. */
+        PERIODIC,
+        OFF         /**< Boundary is turned off. */


Is this really needed, isn't it same as outflow? You can keep one of the above as default case (maybe even outflow). Ofcourse, it is fine as it is, this is just a recommendation.

manishmishra6016 · 2024-06-25T12:04:58Z

+            for (auto &p: cell->getParticles()) {
+                double distanceToBoundary = isLowerBound ? p->getX().at(relevantIndex) : boundaryPosition -
+                                                                                         p->getX().at(relevantIndex);
+                double tolerance = pow(2, 1.0 / 6) * p->getSigma();


possible optimization: you can precompute pow(2, 1.0 / 6) as constexpr and use it here. pow() is expensive, so you can avoid computing it a lot of time.

manishmishra6016 · 2024-06-25T13:29:13Z

+#include "../objects/ParticleContainer.h"
+#include "objects/LinkedCellContainer.h"
+
+class Thermostat {


No documentation. Please add comments, it is hard to understand the code for others and even you in future.

manishmishra6016 · 2024-06-25T13:35:07Z

+    const double currentTemp = calculateCurrentTemp(linkedCellContainer);
+    double beta;
+    if (std::abs(currentTemp) < COMPARISON_TOLERANCE) { // Check if currentTemp is zero
+        beta = sqrt(newTemp / COMPARISON_TOLERANCE); // or some default value


if you don't want user to input init temp as 0, just throw an error here. And if initial temp is not 0, it should never get to 0 (because even in dimensionless case 0 temp corresponds to 0 K, which is improbable and no real physics happens there).

manishmishra6016 · 2024-06-25T13:35:52Z

+#include <vector>
+#include <memory>
+
+class Cell {


no documentation

manishmishra6016 · 2024-06-25T13:37:03Z

+
+#include <vector>
+
+class LinkedCellContainer {


doxygen documentation missing

manishmishra6016 · 2024-06-25T13:38:25Z

+double Particle::getSigma() const {
+    return sigma;
+}
+
+double Particle::getEpsilon() const {
+    return epsilon;


make it inline

laert-ll and others added 30 commits June 12, 2024 15:13

some feedback changes

486eb58

Boundary Implementation

f7f3835

Compilation fix

4f4de78

cmake fix

c80daa1

more fixes

14187b2

complete task 1

ac3f171

activate ci on all assignment branches

90e86cc

preliminary xml setup

dd9814f

xml input for cuboids

afc5f09

restore old changes with FileReader, ParticleGenerator, and parameters

2fe26c2

fix tests

bf7fbab

restore old changes in fileReader and parameters

1d95ac0

restore old changes in fileReader, parameters, ParticleGenerator, Par…

a3a63c2

…ticleGeneratorTest, FileReaderTest

adjust to old changes in FileReader

f8e0c09

use Thermostat in "performSimulation()" with hardcoded args for now

6f676ac

edit xsd

c5d6fb7

update xsd by adding ThermostatParameters

cd98fe4

Refactoring attempt not using BoundaryController

66875e5

Compilation fix

1987779

Merge branch 'refs/heads/Assignment4_Ujin' into Assignment4

7617728

Gravity and some changes in main method with map

8a2ddbf

add and fix test

87da42e

use unique pointers instead of copying objects by value, general fixes

837398c

use shared_ptr for calculator, general fixes

830527c

extend xsd with BoundaryParameters, adjust test

648dd15

adjust XMLReaderTest

751e96c

Refactoring attempt not using BoundaryController

dd26a63

Merge remote-tracking branch 'origin/Assignment4' into Assignment4_Ujin

91dc4d6

# Conflicts: # src/Main.cpp # src/MolSim.h

xml new input format

d92dffe

Fixed BoundaryHandler and MolSim, testing BoundaryHandlerTest.cpp aga…

c3fdb2e

…in but without the periodic tests to check if all the tests except the new ones pass

laert-ll and others added 26 commits June 20, 2024 17:21

added tests for LinkedCellContainer

c92f500

Connect linked-cell algorithm with boundaryHandler

8910294

fix build

94fa06c

do not sanitize, just run tests

917c1f4

fix linkedCellContainerTest

8b5f769

added LC_LJCalculatorTest and adjusted LinkedCellContainerTest

6c4c871

adjustments to LC_LJCalculatorTest

3bd4832

Tests Fix

6009adb

fix "getBoundaryCells()" and add test

833605d

removed calculateX test

7b98293

changed input logic to accept xml file alone

e20c793

add debugging logs, calculateF_LC_pairwise()

6e9e539

added id to particle creation for comparison

1625959

spdlog fix

1c980ec

tiny fix

ea6ae88

add debugging loggings

9d773cc

add sigma and epsilon

bd61e42

add calculateGravityLC in LC_LJCalculator

c8251ed

fix input-disc.xml

5a4514d

upload a template input-cuboid.xml

59dae07

make gravtiy_arg protected

2bc233c

apply gravity to oldF too

6192484

revert applying gravity to oldF too

869bd74

set velocities and forces of ghost particle to zeros

def5566

use downcasting to call calculateFPairwise

b7cbda6

do double operation

bf54952

laert-ll requested review from FG-TUM, manishmishra6016 and thesamriel June 21, 2024 00:39

manishmishra6016 reviewed Jun 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assignment4#34

Assignment4#34
laert-ll wants to merge 108 commits intomainfrom
Assignment4

laert-ll commented Jun 21, 2024

Uh oh!

manishmishra6016 left a comment

Uh oh!

manishmishra6016 Jun 25, 2024

Uh oh!

manishmishra6016 Jun 25, 2024

Uh oh!

manishmishra6016 Jun 25, 2024

Uh oh!

manishmishra6016 Jun 25, 2024

Uh oh!

manishmishra6016 Jun 25, 2024

Uh oh!

manishmishra6016 Jun 25, 2024

Uh oh!

manishmishra6016 Jun 25, 2024

Uh oh!

manishmishra6016 Jun 25, 2024

Uh oh!

manishmishra6016 Jun 25, 2024

Uh oh!

manishmishra6016 Jun 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

laert-ll commented Jun 21, 2024

Uh oh!

manishmishra6016 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants