restructuring consistency check for grids version and restart results file by gauravharsha · Pull Request #10 · Green-Phys/green-sc

gauravharsha · 2026-02-25T05:46:51Z

Idea is to be able to reuse this functionality across other packages and across multiple results files.

…en libraries, and not only as a member function of sc_loop class.

codecov-commenter · 2026-02-25T05:50:38Z

Codecov Report

❌ Patch coverage is 79.16667% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.54%. Comparing base (1082f3d) to head (17bb6f4).

Files with missing lines	Patch %	Lines
src/green/sc/common_defs.h	78.26%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #10      +/-   ##
==========================================
- Coverage   95.65%   95.54%   -0.11%     
==========================================
  Files          10       10              
  Lines         690      696       +6     
==========================================
+ Hits          660      665       +5     
- Misses         30       31       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

This pull request refactors the grids version consistency check functionality to improve reusability. The function check_grids_version_consistency is moved from a private method in sc_loop class to a standalone function in common_defs.h, making it accessible across multiple packages and for checking multiple results files.

Changes:

Moved check_grids_version_consistency from sc_loop.h to common_defs.h as a standalone function
Updated function signature to accept results file path and grids version as parameters
Added comprehensive unit tests for the refactored function

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File	Description
src/green/sc/common_defs.h	Added standalone `check_grids_version_consistency` function with necessary includes
src/green/sc/sc_loop.h	Removed private method implementation, updated constructor to call new standalone function, removed now-unnecessary include
test/sc_test.cpp	Added new test case covering basic scenarios for version consistency checking

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-25T05:51:35Z

src/green/sc/common_defs.h

+   * @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) {


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 · 2026-02-25T05:51:35Z

src/green/sc/common_defs.h

+#include <green/grids/common_defs.h>
+#include <green/h5pp/archive.h>
+#include "except.h"


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 · 2026-02-25T05:51:36Z

src/green/sc/common_defs.h

+   * @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) {


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 · 2026-02-25T05:51:36Z

src/green/sc/common_defs.h

+   * @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) {


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 · 2026-02-25T05:51:36Z

test/sc_test.cpp

+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);
+}


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 · 2026-02-25T05:51:36Z

src/green/sc/common_defs.h

+    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();
+    }


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.

egull

On the technical level: see the codecov and copilot comments. Perhaps add the test that checks for the outdated version, just for bragging rights.

More conceptually this deals with outdated GRIDS, but is part of common_defs in SC. Could or should it be in grids, and perhaps even in grids/common_defs.h? This header seems included anyway, and would be a natural location for grids tests.

gauravharsha · 2026-02-25T06:05:22Z

I agree - the version check functions will be best suited to green-grids. I will refactor the mods and create another pull request. Closing the PR for now.

gauravharsha added 2 commits February 24, 2026 17:41

revise the check_grids_version function for easy use across other gre…

60f233f

…en libraries, and not only as a member function of sc_loop class.

add tests for check_grids_version

17bb6f4

gauravharsha requested review from Copilot and egull February 25, 2026 05:46

Copilot started reviewing on behalf of gauravharsha February 25, 2026 05:47 View session

Copilot AI reviewed Feb 25, 2026

View reviewed changes

egull reviewed Feb 25, 2026

View reviewed changes

gauravharsha closed this Feb 25, 2026

gauravharsha deleted the grids-update branch February 25, 2026 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

restructuring consistency check for grids version and restart results file#10
gauravharsha wants to merge 2 commits intomainfrom
grids-update

gauravharsha commented Feb 25, 2026

Uh oh!

codecov-commenter commented Feb 25, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

Copilot AI Feb 25, 2026

Uh oh!

egull left a comment

Uh oh!

gauravharsha commented Feb 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	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) {

	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) {

Conversation

gauravharsha commented Feb 25, 2026

Uh oh!

codecov-commenter commented Feb 25, 2026

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

egull left a comment

Choose a reason for hiding this comment

Uh oh!

gauravharsha commented Feb 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants