-
Notifications
You must be signed in to change notification settings - Fork 11
378 feature request make sure that results obtained from different benchmark versions are not compared #504
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.
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
base: developing
Are you sure you want to change the base?
Changes from all commits
75fa23d
843b0d5
3d24e11
8088251
df9ad45
1736049
a6db9b6
f38aeb8
c4293be
c4c0ea4
1b8e7f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ | |
| print_code_lib, | ||
| ) | ||
| from jade.helper.constants import CODE | ||
| from jade.helper.errors import VersionInconsistencyError | ||
|
|
||
|
|
||
| class GlobalStatus: | ||
|
|
@@ -36,12 +37,39 @@ def __init__(self, simulations_path: PathLike, raw_results_path: PathLike): | |
| """ | ||
| self.simulations_path = simulations_path | ||
| self.raw_results_path = raw_results_path | ||
| self.update() | ||
|
|
||
| def update(self): | ||
| """Update the status of the simulations and raw results""" | ||
| self.simulations = self._parse_simulations_folder(self.simulations_path) | ||
| self.raw_data = self._parse_raw_results_folder(self.raw_results_path) | ||
| self._simulations = None | ||
| self._raw_data = None | ||
| self._raw_metadata = None | ||
|
|
||
| @property | ||
| def simulations(self) -> dict[tuple[CODE, str, str], CodeLibRunStatus]: | ||
| if self._simulations is None: | ||
| self._simulations = self._parse_simulations_folder(self.simulations_path) | ||
| return self._simulations | ||
|
Comment on lines
+46
to
+48
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a refresh path for cached simulation state. After the first Possible fix+ def update_simulations(self) -> None:
+ """Update the simulations by re-parsing the simulations folder."""
+ self._simulations = self._parse_simulations_folder(self.simulations_path)
+
def update_raw_results(self) -> None:
"""Update the raw results by re-parsing the raw results folder. It should be used
after processing new raw results to update the status.
"""
self._raw_data, self._raw_metadata = self._parse_raw_results_folder(
self.raw_results_path
)Also applies to: 66-72 🤖 Prompt for AI Agents |
||
|
|
||
| @property | ||
| def raw_data(self) -> dict[tuple[CODE, str, str], list[str]]: | ||
| if self._raw_data is None: | ||
| self._raw_data, self._raw_metadata = self._parse_raw_results_folder( | ||
| self.raw_results_path | ||
| ) | ||
|
Comment on lines
+52
to
+55
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make raw-data discovery tolerant to missing metadata, then fail cleanly during validation.
Possible fix- available_raw_data[(CODE(code), lib, benchmark)] = os.listdir(
- bench_path
- )
+ available_raw_data[(CODE(code), lib, benchmark)] = [
+ entry for entry in os.listdir(bench_path)
+ if entry != "metadata.json"
+ ]
if lib != "exp": # for the experiments we don't have metadata
- with open(os.path.join(bench_path, "metadata.json")) as infile:
- metadata[(CODE(code), lib, benchmark)] = json.load(infile)
+ metadata_file = Path(bench_path, "metadata.json")
+ if metadata_file.is_file():
+ with metadata_file.open() as infile:
+ metadata[(CODE(code), lib, benchmark)] = json.load(infile)
...
versions = {}
for lib in libs:
- versions[lib] = self.raw_metadata[(code, lib, benchmark)][
- "benchmark_version"
- ]
+ version = self.raw_metadata.get((code, lib, benchmark), {}).get(
+ "benchmark_version"
+ )
+ if version is None:
+ raise VersionInconsistencyError(
+ f"Missing benchmark_version metadata for {code} / {lib} / {benchmark}"
+ )
+ versions[lib] = versionAlso applies to: 60-63, 144-146, 292-294 🤖 Prompt for AI Agents |
||
| return self._raw_data | ||
|
|
||
| @property | ||
| def raw_metadata(self) -> dict[tuple[CODE, str, str], dict]: | ||
| if self._raw_metadata is None: | ||
| self._raw_data, self._raw_metadata = self._parse_raw_results_folder( | ||
| self.raw_results_path | ||
| ) | ||
| return self._raw_metadata | ||
|
|
||
| def update_raw_results(self) -> None: | ||
| """Update the raw results by re-parsing the raw results folder. It should be used | ||
| after processing new raw results to update the status. | ||
| """ | ||
| self._raw_data, self._raw_metadata = self._parse_raw_results_folder( | ||
| self.raw_results_path | ||
| ) | ||
|
|
||
| def _parse_simulations_folder( | ||
| self, simulations_path: PathLike | ||
|
|
@@ -65,7 +93,9 @@ def _parse_simulations_folder( | |
| for sub_bench in os.listdir(bench_path): | ||
| # check if the run was successful | ||
| sub_bench_path = Path(bench_path, sub_bench) | ||
| success = CODE_CHECKERS[code](sub_bench_path) | ||
| success = CODE_CHECKERS[code].check_success( | ||
| os.listdir(sub_bench_path) | ||
| ) | ||
| if success: | ||
| successful.append(sub_bench) | ||
| else: | ||
|
|
@@ -93,9 +123,12 @@ def _parse_simulations_folder( | |
|
|
||
| def _parse_raw_results_folder( | ||
| self, path_raw: PathLike | ||
| ) -> dict[tuple[CODE, str, str], list[str]]: | ||
| ) -> tuple[ | ||
| dict[tuple[CODE, str, str], list[str]], dict[tuple[CODE, str, str], dict] | ||
| ]: | ||
| # simply store a dictionary with the processed raw results | ||
| available_raw_data = {} | ||
| metadata = {} | ||
| for code_lib in os.listdir(path_raw): | ||
| codelib_path = Path(path_raw, code_lib) | ||
| if not codelib_path.is_dir(): | ||
|
|
@@ -108,7 +141,10 @@ def _parse_raw_results_folder( | |
| available_raw_data[(CODE(code), lib, benchmark)] = os.listdir( | ||
| bench_path | ||
| ) | ||
| return available_raw_data | ||
| if lib != "exp": # for the experiments we don't have metadata | ||
| with open(os.path.join(bench_path, "metadata.json")) as infile: | ||
| metadata[(CODE(code), lib, benchmark)] = json.load(infile) | ||
| return available_raw_data, metadata | ||
|
|
||
| def was_simulated(self, code: CODE, lib: str, benchmark: str) -> bool: | ||
| """Check if a simulation was already performed and if it was successful. | ||
|
|
@@ -230,6 +266,63 @@ def is_raw_available(self, codelib: str, benchmark: str) -> bool: | |
| return True | ||
| return False | ||
|
|
||
| def _validate_libs_processing( | ||
| self, code: CODE, benchmark: str, libs: list[str] | ||
| ) -> None: | ||
| """Check that the post-processing can be performed for the given code and | ||
| benchmark. This is true if the benchmark version for the requested libs | ||
| are the same. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| code : CODE | ||
| code used in the simulation. | ||
| benchmark : str | ||
| benchmark name. | ||
| libs : list[str] | ||
| list of libraries to check. | ||
|
|
||
| Returns | ||
| ------- | ||
| bool | ||
| True if the post-processing can be performed, False otherwise. | ||
|
Comment on lines
+285
to
+288
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Docstring incorrectly states return type. The docstring states "Returns -> bool: True if the post-processing can be performed" but the method returns 📝 Proposed docstring fix- Returns
- -------
- bool
- True if the post-processing can be performed, False otherwise.
+ Raises
+ ------
+ VersionInconsistencyError
+ If the benchmark versions are not consistent across the requested libraries.🤖 Prompt for AI Agents |
||
| """ | ||
| versions = {} | ||
| for lib in libs: | ||
| # I want only the major version | ||
| versions[lib] = self.raw_metadata[(code, lib, benchmark)][ | ||
| "benchmark_version" | ||
| ].split(".")[0] | ||
| if len(set(versions.values())) > 1: | ||
| raise VersionInconsistencyError( | ||
| f"The versions of the requested libraries for {code} and benchmark {benchmark} are not consistent: {versions}" | ||
| ) | ||
|
|
||
| def validate_codelibs( | ||
| self, codelibs: list[tuple[CODE, str]], benchmarks: str | ||
| ) -> None: | ||
| """Check that a list of codelibs can be post-processed together for a given benchmark. | ||
| This is true if the benchmark version for the requested libs are the same. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| codelibs : list[tuple[CODE, str]] | ||
| list of codelibs to check. Each codelib is a tuple with the code and library. | ||
| benchmark : str | ||
| benchmark name. | ||
|
Comment on lines
+301
to
+312
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Parameter name and docstring inconsistency. The parameter is named 📝 Proposed fix def validate_codelibs(
- self, codelibs: list[tuple[CODE, str]], benchmarks: str
+ self, codelibs: list[tuple[CODE, str]], benchmark: str
) -> None:
"""Check that a list of codelibs can be post-processed together for a given benchmark.
...
for code, lib_list in libs.items():
- self._validate_libs_processing(code, benchmarks, lib_list)
+ self._validate_libs_processing(code, benchmark, lib_list)🤖 Prompt for AI Agents |
||
| """ | ||
| libs = {} | ||
| for code, lib in codelibs: | ||
| code = CODE(code) | ||
| if lib == "exp": # for the experiments we don't have metadata | ||
| continue | ||
| if code not in libs: | ||
| libs[code] = [] | ||
| libs[code].append(lib) | ||
|
|
||
| for code, lib_list in libs.items(): | ||
| self._validate_libs_processing(code, benchmarks, lib_list) | ||
|
|
||
|
|
||
| @dataclass | ||
| class CodeLibRunStatus: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| import importlib.metadata | ||
| import os | ||
| import re | ||
| from abc import ABC, abstractmethod | ||
| from pathlib import Path | ||
| from typing import TYPE_CHECKING, Union | ||
|
|
||
|
|
@@ -68,32 +69,61 @@ def print_code_lib(code: CODE, lib: Library | str, pretty: bool = False) -> str: | |
| return f"_{code.value}_-_{lib_name}_" | ||
|
|
||
|
|
||
| def check_run_mcnp(folder: PathLike) -> bool: | ||
| """check if mcnp run was successful""" | ||
| try: | ||
| MCNPSimOutput.retrieve_files(folder) | ||
| return True | ||
| except FileNotFoundError: | ||
| return False | ||
| class SimulationChecker(ABC): | ||
| """Abstract base class for simulation success checkers. | ||
|
|
||
| All code-specific checkers must inherit from this class and implement | ||
| the check_success method with the same signature. | ||
| """ | ||
|
|
||
| def check_run_openmc(folder: PathLike) -> bool: | ||
| """check if openmc run was successful""" | ||
| try: | ||
| OpenMCSimOutput.retrieve_file(folder) | ||
| return True | ||
| except FileNotFoundError: | ||
| return False | ||
| @abstractmethod | ||
| def check_success(self, files: list[str]) -> bool: | ||
| """Check if a simulation run was successful. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| files : list[str] | ||
| List of output files to check for success. | ||
|
|
||
| Returns | ||
| ------- | ||
| bool | ||
| True if the simulation completed successfully, False otherwise. | ||
| """ | ||
| pass | ||
|
|
||
|
|
||
| class MCNPChecker(SimulationChecker): | ||
| """Checker for MCNP simulations.""" | ||
|
|
||
| def check_success(self, files: list[str]) -> bool: | ||
| """Check if MCNP run was successful by verifying output files exist.""" | ||
| return MCNPSimOutput.is_successfully_simulated(files) | ||
|
|
||
|
|
||
| class OpenMCChecker(SimulationChecker): | ||
| """Checker for OpenMC simulations.""" | ||
|
|
||
| def check_success(self, files: list[str]) -> bool: | ||
| """Check if OpenMC run was successful by verifying output files exist.""" | ||
| return OpenMCSimOutput.is_successfully_simulated(files) | ||
|
|
||
|
|
||
| class SerpentChecker(SimulationChecker): | ||
| """Checker for Serpent simulations.""" | ||
|
|
||
| def check_success(self, files: list[str]) -> bool: | ||
| """Check if Serpent run was successful.""" | ||
| # TODO implement the logic to check if the Serpent run was successful | ||
| raise NotImplementedError("Serpent checker not yet implemented") | ||
|
|
||
| def check_run_serpent(folder: PathLike) -> bool: | ||
| # TODO implement the logic to check if the Serpent run was successful | ||
| raise NotImplementedError | ||
|
|
||
| class D1SChecker(SimulationChecker): | ||
| """Checker for D1S simulations.""" | ||
|
|
||
| def check_run_d1s(folder: PathLike) -> bool: | ||
| """check if d1s run was successful""" | ||
| return check_run_mcnp(folder) | ||
| def check_success(self, files: list[str]) -> bool: | ||
| """Check if D1S run was successful (uses same logic as MCNP).""" | ||
| return MCNPChecker().check_success(files) | ||
|
Comment on lines
+72
to
+126
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keep the run directory in The new 💡 Suggested interface direction class SimulationChecker(ABC):
"""Abstract base class for simulation success checkers.
@@
`@abstractmethod`
- def check_success(self, files: list[str]) -> bool:
+ def check_success(
+ self, run_dir: PathLike, files: list[str] | None = None
+ ) -> bool:
"""Check if a simulation run was successful.🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| def get_jade_version() -> str: | ||
|
|
@@ -124,11 +154,13 @@ def add_rmode0(path: PathLike) -> None: | |
| f.write("RMODE 0\n") | ||
|
|
||
|
|
||
| CODE_CHECKERS = { | ||
| CODE.MCNP: check_run_mcnp, | ||
| CODE.OPENMC: check_run_openmc, | ||
| CODE.SERPENT: check_run_serpent, | ||
| CODE.D1S: check_run_d1s, | ||
| # Dictionary mapping CODE enums to checker instances | ||
| # All checkers implement the SimulationChecker interface | ||
| CODE_CHECKERS: dict[CODE, SimulationChecker] = { | ||
| CODE.MCNP: MCNPChecker(), | ||
| CODE.OPENMC: OpenMCChecker(), | ||
| CODE.SERPENT: SerpentChecker(), | ||
| CODE.D1S: D1SChecker(), | ||
| } | ||
|
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invalidate the cached
GlobalStatusafter write operations.This property memoizes one
GlobalStatussnapshot for the lifetime ofJadeApp. Afterrun_benchmarks()orraw_process()changes the simulations/raw trees, later calls still consult stalesimulations/raw_dataunless callers refresh manually —tests/app/test_app.pyLines 63-72 now has to do that explicitly. A same-sessionraw_process()→post_process()flow can therefore miss the raw results it just produced.🤖 Prompt for AI Agents