Skip to content

restructuring consistency check for grids version and restart results file #10

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
gauravharsha wants to merge 2 commits into from
Closed

restructuring consistency check for grids version and restart results file #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
60f233f
revise the check_grids_version function for easy use across other gre…
gauravharsha Feb 24, 2026
17bb6f4
add tests for check_grids_version
gauravharsha Feb 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions src/green/sc/common_defs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@

#include <green/ndarray/ndarray.h>
#include <green/params/params.h>
#include <green/grids/common_defs.h>
#include <green/h5pp/archive.h>
#include "except.h"
Comment on lines +26 to +28
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function uses std::filesystem::exists but there is no #include directive in this file. While this may work due to transitive includes from other headers, it creates a fragile dependency. The file should explicitly include to ensure portability and make dependencies clear.

Copilot uses AI. Check for mistakes.

namespace green::sc {
/**
Expand Down Expand Up @@ -113,5 +116,51 @@ namespace green::sc {

return 0;
}

/**
* @brief Checks consistency between grid-file version used in current run vs. the version
* used to generate the results file that is being (re-)started from.
* This is to prevent users from accidentally restarting from a results file that was generated with
* an older version of green-grids, which can lead to silent errors in the results.
*
* 1. If the results file does not have a green-grids version attribute, treat it as having been
* generated with the baseline grids version (GRIDS_MIN_VERSION, currently 0.2.4) and check
* that the current grid-file version is not newer; otherwise an error is raised.
* 2. If the results file has a green-grids version attribute, compare that version with the current
* grid-file version and distinguish older, equal, and newer cases, allowing only compatible
* combinations and throwing specific errors when there is a mismatch.
*
* @param results_file - path to the results file that is being restarted from
* @param grid_file_version - version of green-grids used in the current run (can be obtained from DysonSolver::get_grids_version())
*/
void check_grids_version_consistency(std::string results_file, const std::string grid_file_version) {
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first parameter results_file should be passed by const reference instead of by value to avoid unnecessary string copying. This is especially important for functions that may be called frequently or with potentially long file paths.

Suggested change
void check_grids_version_consistency(std::string results_file, const std::string grid_file_version) {
void check_grids_version_consistency(const std::string& results_file, const std::string& grid_file_version) {

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second parameter name grid_file_version is inconsistent with how it's documented. The documentation says "version of green-grids used in the current run" but the parameter is named grid_file_version. Consider renaming to grids_version or current_grids_version for clarity and consistency with the terminology used throughout the function (e.g., "green-grids version").

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function is defined in a header file but is not marked as inline. This can lead to multiple definition linker errors if the header is included in multiple translation units. Following the pattern of other functions in this file (like define_parameters and compare_version_strings), this function should be marked as inline.

Suggested change
void check_grids_version_consistency(std::string results_file, const std::string grid_file_version) {
inline void check_grids_version_consistency(std::string results_file, const std::string grid_file_version) {

Copilot uses AI. Check for mistakes.
// If results file does not exist, nothing to check
if (!std::filesystem::exists(results_file)) return;

h5pp::archive ar(results_file, "r");
if (ar.has_attribute("__grids_version__")) {
std::string grids_version_in_results;
grids_version_in_results = ar.get_attribute<std::string>("__grids_version__");
ar.close(); // safely close before throwing
if (compare_version_strings(grid_file_version, grids_version_in_results) < 0) {
throw green::grids::outdated_grids_file_error("The current green-grids version (" + grid_file_version +
") is older than the green-grids version used to create the original results file ("
+ grids_version_in_results +
"). Please update green-grids to version " + grids_version_in_results);
} else if (compare_version_strings(grid_file_version, grids_version_in_results) > 0) {
throw outdated_results_file_error("The green-grids version used to create the results file (" + grids_version_in_results +
") is older than the current specified grid file (" + grid_file_version +
"). Please download the appropriate version from: " +
"https://github.com/Green-Phys/green-grids/releases/ or https://github.com/Green-Phys/green-grids/tags");
}
} else if (compare_version_strings(grid_file_version, green::grids::GRIDS_MIN_VERSION) > 0) {
ar.close(); // safely close before throwing
throw outdated_results_file_error("The results file was created using un-versioned grid file (equiv. to " + green::grids::GRIDS_MIN_VERSION +
") and the current green-grids version (" + grid_file_version + ") is newer.\n" +
"Please use old grid files from: https://github.com/Green-Phys/green-grids/releases/tag/v0.2.4.");
} else {
ar.close();
}
Comment on lines +141 to +163
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a control flow bug: when the file has a grids_version attribute and the versions are equal (compatible case), the archive is closed at line 144, then execution falls through to line 156. If the condition at line 156 is also false, execution reaches line 162 which attempts to close the archive again. This double-close could cause errors depending on the h5pp::archive implementation. The function should return after line 155 when versions are compatible, or restructure the logic to avoid attempting to close an already-closed archive.

Copilot uses AI. Check for mistakes.
}
} // namespace green::sc
#endif // GREEN_SC_COMMON_DEFS_H
49 changes: 1 addition & 48 deletions src/green/sc/sc_loop.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
#include <green/params/params.h>
#include <green/utils/mpi_utils.h>
#include <green/utils/timing.h>
#include <green/grids/common_defs.h>
#include <mpi.h>

#include <cstdio>
Expand Down Expand Up @@ -78,7 +77,7 @@ namespace green::sc {
_itermax(p["itermax"]), _iter(0), _e_thr(p["threshold"]), _e_thr_sp(p["E_thr_sp"]), _input_path(p["input_file"]),
_results_file(p["results_file"]), _restart(p["restart"]), _dyson_solver(p), _mix(p), _context(comm) {
if (_restart) {
check_grids_version_consistency();
check_grids_version_consistency(_results_file, _dyson_solver.get_grids_version());
}
}

Expand Down Expand Up @@ -182,52 +181,6 @@ namespace green::sc {
ar.close();
}

/**
* @brief Checks consistency between grid-file version used in current run vs. the version
* used to generate the results file that is being restarted from.
* This is to prevent users from accidentally restarting from a results file that was generated with
* an older version of green-grids, which can lead to silent errors in the results.
*
* 1. If the results file does not have a green-grids version attribute, treat it as having been
* generated with the baseline grids version (GRIDS_MIN_VERSION, currently 0.2.4) and check
* that the current grid-file version is not newer; otherwise an error is raised.
* 2. If the results file has a green-grids version attribute, compare that version with the current
* grid-file version and distinguish older, equal, and newer cases, allowing only compatible
* combinations and throwing specific errors when there is a mismatch.
*/
void check_grids_version_consistency() {
// If results file does not exist, nothing to check
if (!std::filesystem::exists(_results_file)) return;

// Read grid-file version
const std::string& grid_file_version = _dyson_solver.get_grids_version();

h5pp::archive ar(_results_file, "r");
if (ar.has_attribute("__grids_version__")) {
std::string grids_version_in_results;
grids_version_in_results = ar.get_attribute<std::string>("__grids_version__");
ar.close(); // safely close before throwing
if (compare_version_strings(grid_file_version, grids_version_in_results) < 0) {
throw green::grids::outdated_grids_file_error("The current green-grids version (" + grid_file_version +
") is older than the green-grids version used to create the original results file ("
+ grids_version_in_results +
"). Please update green-grids to version " + grids_version_in_results);
} else if (compare_version_strings(grid_file_version, grids_version_in_results) > 0) {
throw outdated_results_file_error("The green-grids version used to create the results file (" + grids_version_in_results +
") is older than the current specified grid file (" + grid_file_version +
"). Please download the appropriate version from: " +
"https://github.com/Green-Phys/green-grids/releases/ or https://github.com/Green-Phys/green-grids/tags");
}
} else if (compare_version_strings(grid_file_version, green::grids::GRIDS_MIN_VERSION) > 0) {
ar.close(); // safely close before throwing
throw outdated_results_file_error("The results file was created using un-versioned grid file (equiv. to " + green::grids::GRIDS_MIN_VERSION +
") and the current green-grids version (" + grid_file_version + ") is newer.\n" +
"Please use old grid files from: https://github.com/Green-Phys/green-grids/releases/tag/v0.2.4.");
} else {
ar.close();
}
}

DysonSolver& dyson_solver() { return _dyson_solver; }
};
} // namespace green::sc
Expand Down
18 changes: 18 additions & 0 deletions test/sc_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,24 @@ TEST_CASE("FockSigmaVectorSpace") { test_vector_space<green::opt::vector_space_f

TEST_CASE("CyclicFockSigmaVectorSpace") { test_vector_space<green::opt::cyclic_vector_space_fock_sigma>(); }

TEST_CASE("Grids Version Consistency") {
// 1. Starting with new file (does not exist).
// Version check should pass because there's nothing to check / no inconsistency
std::string res_file_1 = random_name();
REQUIRE_NOTHROW(green::sc::check_grids_version_consistency(res_file_1, "0.2.4"));

// 2. Open the file and add __grids_version__ attribute
green::h5pp::archive ar_res_1(res_file_1, "w");
ar_res_1.set_attribute<std::string>("__grids_version__", "0.2.4");
ar_res_1.close();
REQUIRE_NOTHROW(green::sc::check_grids_version_consistency(res_file_1, green::grids::GRIDS_MIN_VERSION));
REQUIRE_THROWS_AS(green::sc::check_grids_version_consistency(res_file_1, "0.2.3"),
green::grids::outdated_grids_file_error);
REQUIRE_THROWS_AS(green::sc::check_grids_version_consistency(res_file_1, "0.2.5"),
green::sc::outdated_results_file_error);
std::filesystem::remove(res_file_1);
}
Comment on lines +637 to +653
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test case does not cover the scenario where the results file exists but lacks a grids_version attribute. This is an important edge case handled in the function at lines 156-160 of common_defs.h. Consider adding a test section that creates a file without the version attribute and verifies that the function throws outdated_results_file_error when the current version is newer than GRIDS_MIN_VERSION.

Copilot uses AI. Check for mistakes.

int main(int argc, char** argv) {
MPI_Init(&argc, &argv);
int result = Catch::Session().run(argc, argv);
Expand Down