Conversation
add pdf ref
Fix formatting of code block in README.
Updated the CSV format to use commas instead of semicolons for better compatibility.
There was a problem hiding this comment.
Pull request overview
This is a project submission that implements a parallel PDE (Partial Differential Equation) solver with adaptive mesh refinement and adaptive time stepping capabilities. The implementation uses deal.II library with MPI parallelization.
- The PR adds a complete C++ implementation of a heat equation solver with space and time adaptivity
- Includes Python scripts for benchmarking, scalability analysis, and accuracy testing
- Provides results data including scalability metrics and performance plots
Reviewed changes
Copilot reviewed 21 out of 28 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| cpp_program/src/Heat.cpp | Core implementation of the heat equation solver with adaptive methods |
| cpp_program/src/Heat.hpp | Header file defining the Heat class and related functions |
| cpp_program/src/main.cpp | Entry point for the application |
| cpp_program/parameters_base.prm | Configuration file for solver parameters |
| scripts/scalability_MN.slurm | SLURM batch script for scalability testing on MareNostrum cluster |
| scripts/analyze_scalability.py | Python script to analyze and visualize scalability results |
| scripts/accuracy_benchmark.py | Python script to run accuracy benchmarks |
| cpp_program/tests/*.py | Testing utilities and benchmark scripts |
| results/.csv, results/.png | Result data and visualization files |
| pdeEnv.yml | Conda environment specification |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for line in lines: | ||
| stripped_line = line.strip() | ||
|
|
||
| # Controlla i cambi di stato (inizio o fine di una sottosezione) |
There was a problem hiding this comment.
The comment "Controlla i cambi di stato" is in Italian. Comments should be in English for consistency and to ensure all team members can understand them.
| modified_lines.append(line) | ||
| continue | ||
|
|
||
| # Se siamo in una sottosezione di interesse, prova a modificare i parametri |
There was a problem hiding this comment.
The comment "Se siamo in una sottosezione di interesse, prova a modificare i parametri" is in Italian. Comments should be in English for consistency.
| # Cerca una riga che inizi con "set", seguita dal nome del parametro | ||
| pattern = re.compile(f"^\\s*set\\s+{re.escape(param)}\\s*=", re.IGNORECASE) | ||
| if pattern.search(stripped_line): | ||
| # Ricostruisce la riga per preservare l'indentazione e formattare correttamente | ||
| indentation = re.match(r"(\s*)", line).group(1) | ||
| new_line = f"{indentation}set {param} = {value}\n" | ||
| break # Parametro trovato e sostituito, passa alla riga successiva |
There was a problem hiding this comment.
Multiple Italian comments appear throughout this section. Comments should be in English for consistency. Lines contain: "Cerca una riga che inizi con", "Ricostruisce la riga per preservare l'indentazione", and "Parametro trovato e sostituito, passa alla riga successiva".
| }double | ||
|
|
||
| Heat::get_sigma_min_from_prm(ParameterHandler &prm) |
There was a problem hiding this comment.
There's a formatting issue with the function declaration. The double return type appears on line 160 which is separate from the function name Heat::get_sigma_min_from_prm on line 162. This breaks the standard function declaration format and makes the code harder to read.
There was a problem hiding this comment.
I second this comment, and suggest you to use automatic formatting tools such as clang-format, which should be able to catch this sort of thing.
| }double | ||
|
|
||
| Heat::get_amplitude_min_from_prm(ParameterHandler &prm) |
There was a problem hiding this comment.
Similar formatting issue as above. The double return type appears on line 177 separately from the function name on line 179.
| Usage: python analyze_scalability.py <results_directory> | ||
| """ | ||
|
|
||
| import os |
There was a problem hiding this comment.
Import of 'os' is not used.
| import os |
| import subprocess | ||
| import re | ||
| import os | ||
| import sys |
There was a problem hiding this comment.
Import of 'sys' is not used.
| import sys |
|
|
||
| import subprocess | ||
| import re | ||
| import os |
There was a problem hiding this comment.
Import of 'os' is not used.
| import os |
|
|
||
| import subprocess | ||
| import re | ||
| import os |
There was a problem hiding this comment.
Import of 'os' is not used.
| import os |
| wget https://github.com/Kitware/CMake/releases/download/v3.25.0/cmake-3.25.0.tar.gz | ||
| tar -xf cmake-3.25.0.tar.gz | ||
| mkdir cmake-3.25.0-build && cd cmake-3.25.0-build | ||
| ../cmake-3.25.0/configure --prefix=/usr/local | ||
| make -j$(nproc) | ||
| sudo make install | ||
|
|
||
| # 3. IMPOSTA L'AMBIENTE PER LA BUILD | ||
| export PATH="/opt/mpi/bin:$PATH" | ||
| export LD_LIBRARY_PATH="/opt/mpi/lib:$LD_LIBRARY_PATH" | ||
|
|
||
| export CC=mpicc | ||
| export CXX=mpic++ | ||
| export FC=mpif90 | ||
|
|
||
| # 4. INSTALLA TRILINOS | ||
| # Questa è una dipendenza di deal.II per l'algebra lineare parallela. | ||
| # La configurazione abilita solo i pacchetti minimi necessari a deal.II. | ||
| git clone --depth 1 --branch trilinos-release-14-4-0 https://github.com/trilinos/Trilinos.git /trilinos_source | ||
| mkdir /trilinos_build; cd /trilinos_build | ||
| cmake -D CMAKE_INSTALL_PREFIX=/opt/trilinos \ | ||
| -D CMAKE_BUILD_TYPE=Release \ | ||
| -DCMAKE_POSITION_INDEPENDENT_CODE=ON \ | ||
| -D Trilinos_ENABLE_ALL_OPTIONAL_PACKAGES=OFF \ | ||
| -D Trilinos_ENABLE_CXX11=ON \ | ||
| -D Trilinos_ENABLE_Epetra=ON \ | ||
| -D Trilinos_ENABLE_EpetraExt=ON \ | ||
| -D Trilinos_ENABLE_Ifpack=ON \ | ||
| -D Trilinos_ENABLE_AztecOO=ON \ | ||
| -D Trilinos_ENABLE_Amesos=ON \ | ||
| -D Trilinos_ENABLE_ML=ON \ | ||
| -D TPL_ENABLE_MPI=ON \ | ||
| /trilinos_source | ||
| make -j$(nproc) install | ||
| cd /; rm -rf /trilinos_source /trilinos_build | ||
|
|
||
| # 5. INSTALLA DEAL.II CON SUPPORTO A TRILINOS | ||
| # Ora diciamo a deal.II di usare la versione di Trilinos che abbiamo installato. | ||
| git clone --depth 1 --branch v9.5.1 https://github.com/dealii/dealii.git /dealii_source | ||
| mkdir /dealii_build; cd /dealii_build | ||
| cmake -D CMAKE_INSTALL_PREFIX=/opt/dealii \ | ||
| -D DEAL_II_WITH_MPI=ON \ | ||
| -D DEAL_II_WITH_TRILINOS=ON \ | ||
| -D Trilinos_DIR=/opt/trilinos/lib/cmake/Trilinos \ | ||
| -D DEAL_II_WITH_TBB=ON \ | ||
| -D DEAL_II_WITH_EIGEN=ON \ | ||
| -D DEAL_II_WITH_P4EST=ON \ | ||
| -D CMAKE_BUILD_TYPE=Release \ | ||
| /dealii_source | ||
| make -j$(nproc) install | ||
| cd /; rm -rf /dealii_source /dealii_build |
There was a problem hiding this comment.
The %post section downloads and builds tooling and libraries (cmake, Trilinos, deal.II) directly from remote GitHub URLs using wget and git clone without any integrity verification, and executes the resulting code as root during the container build. If an attacker can tamper with DNS, TLS, or the referenced tags, they can inject arbitrary code into your build pipeline via these mutable references. To mitigate this supply chain risk, pin these dependencies to immutable identifiers (e.g., specific commit SHAs) and verify release artifacts using cryptographic checksums or signatures before building and installing them inside the container.
|
Please include your names in the description of the project. I can probably guess who Crippius is, but I'd rather not to. Please remember that this is the delivery of a project for an exam. Behave and communicate with the appropriate formality. |
|
Dear Professor Bucelli, We apologize for the oversight, we initially thought that including our names in the README was sufficient. We have now updated the Pull Request description to include our full names alongside our GitHub usernames to ensure there is no ambiguity. Best regards, |
| ParameterHandler prm; | ||
|
|
||
| Heat::declare_parameters(prm); | ||
| prm.parse_input(argv[1]); |
There was a problem hiding this comment.
Although it's probably a bit overkill for this particular case, you might want to look into more structured ways of dealing with command line arguments. One example tool is clipp.
| // Spatial parameters (N_x) | ||
| std::vector<double> a; // amplitudes | ||
| std::vector<Point<dim>> c; // centers | ||
| std::vector<double> sigma; // widths | ||
|
|
||
| // Temporal parameters (N_t) | ||
| std::vector<double> b; // amplitudes | ||
| std::vector<double> tau; // centers | ||
| std::vector<double> delta; // widths |
There was a problem hiding this comment.
These should most definitely be part of the private or protected scope of the class, since they are supposed to be set by the constructor and never modified again. For the same reason, they should be declared as const.
| }double | ||
|
|
||
| Heat::get_sigma_min_from_prm(ParameterHandler &prm) |
There was a problem hiding this comment.
I second this comment, and suggest you to use automatic formatting tools such as clang-format, which should be able to catch this sort of thing.
| system_rhs); | ||
| } | ||
|
|
||
| t0 = std::chrono::high_resolution_clock::now(); |
There was a problem hiding this comment.
Have you considered using dealii::TimerOutput for this?
Project Submission
Space and Time adaptivity
Numerical Methods for Partial Differential Equations – Project 2025/26 - Politecnico di Milano
Students: