Update reporting / figures #10

imallona · 2025-12-11T10:46:09Z

Copilot

Pull request overview

This PR updates the results parsing and reporting infrastructure to support the new naming convention that includes seed and run identifiers in output directory names. The changes shift from the old out-{backend}-{rep} format to out_{backend}_seed_{seed}_run_{run} format and introduce a new R Markdown analysis file for Izaskun's reporting needs.

Key changes:

Updated path parsing regex patterns to extract backend, seed, and run from the new directory naming format
Added new R Markdown report (analyze_results_izaskun.Rmd) with comprehensive performance and metrics visualization
Modified Makefile to redirect parse_results.py output to JSON file and render the new report
Removed credentials file .play_minio.json

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
parse_results.py	Updated regex patterns to parse new directory naming convention with seed/run identifiers; includes large commented-out code block and a critical regex bug on line 158
analyze_results_izaskun.Rmd	New R Markdown file for comprehensive performance analysis and visualization across backends
Makefile	Updated `knit_report` target to output JSON file and render new analysis report; commented out old report generation
.play_minio.json	Removed credentials file (security improvement)

Comments suppressed due to low confidence (1)

parse_results.py:52

This comment appears to contain commented-out code.

# def parse_result_path(path: Path) -> Dict[str, str]:
#     """
#     Parse a result path and extract components.
    
#     """
#     parts = path.parts

#     # print(pars)
    
#     result = {}

#     # Parse out-{backend}-{rep}
#     out_match = re.match( r"out_(?P<backend>[a-zA-Z0-9]+)_seed_(?P<seed>\d+)_run_(?P<run>\d+)", parts)
#     if out_match:
#         result['backend'] = out_match.group(1)
#         result['seed'] = out_match.group(2)
#         result['rep'] = out_match.group(3)

#     # Find dataset_generator part
#     for part in parts:
#         if part.startswith('dataset_generator-'):
#             # Parse dataset_generator-{generator}_dataset_name-{name}
#             dataset_match = re.match(r'dataset_generator-([^_]+)_dataset_name-(.+)', part)
#             if dataset_match:
#                 result['generator'] = dataset_match.group(1)
#                 result['dataset_name'] = dataset_match.group(2)
#             break

#     # The method is the last part (after clustering/)
#     if 'clustering' in parts:
#         clustering_idx = parts.index('clustering')
#         if clustering_idx + 1 < len(parts):
#             result['method'] = parts[clustering_idx + 1]

#     result['path'] = str(path)

#     return result

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

parse_results.py

Copilot · 2025-12-11T16:48:55Z

analyze_results_izaskun.Rmd

+```{r, fig.height = 10, fig.width = 10}
+
+ggplot(fd, aes(x = factor(k), y = metric_value, fill = method)) +
+  geom_jitter(width = 0.2, alpha = 0.4, size = 1, fill = "black") +


The geom_jitter call has fill = "black" which is incorrect for point geometries. Points use color for their appearance, not fill. The fill aesthetic applies to shapes with interior areas (like polygons or certain point shapes).

This should be color = "black" to properly style the jittered points.

Suggested change

geom_jitter(width = 0.2, alpha = 0.4, size = 1, fill = "black") +

geom_jitter(width = 0.2, alpha = 0.4, size = 1, color = "black") +

parse_results.py

analyze_results_izaskun.Rmd

…ng Bland Altmans ks vs true k

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

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

parse_results.py

Copilot · 2025-12-15T14:09:06Z

parse_results.py

+            # Normalize method name
+            m = re.match(r"method-([^_]+)", config_dir.name)
+            if m:
+                record["method"] = m.group(1)


The method name extraction logic may incorrectly override the method field. On line 236, if a match is found in config_dir.name, it sets record["method"]. However, record already has a "method" field from base_result (set on line 58). If the config directory has a different method name pattern than expected, this could lead to inconsistent method names. Consider checking if the extracted method matches the existing one or using a different field name to avoid confusion.

Suggested change

record["method"] = m.group(1)

method_from_dir = m.group(1)

if "method" in record and record["method"] != method_from_dir:

record["method_from_dir"] = method_from_dir

# else: do not overwrite record["method"]

Copilot · 2025-12-15T14:09:07Z

parse_results.py

+            if config_dir.name not in variant_name:
+                record["method_full"] = f"{variant_name}_{config_dir.name}"


The method_full construction logic has a potential issue. On lines 239-241, the code checks if config_dir.name is not in variant_name before appending it. However, variant_name comes from record["method_full"] which was set on line 75 as f"{method_dir}_{child.name}". Then on line 241, it appends config_dir.name again. This could result in duplicate directory names in the method_full string if config_dir.name equals child.name, or create an incorrect hierarchy. Consider reviewing the logic to ensure method_full is constructed consistently.

Suggested change

if config_dir.name not in variant_name:

record["method_full"] = f"{variant_name}_{config_dir.name}"

# Only append config_dir.name if it's not already the last component

if not variant_name.split('_')[-1] == config_dir.name:

record["method_full"] = f"{variant_name}_{config_dir.name}"

else:

record["method_full"] = variant_name

Makefile

analyze_results_izaskun.Rmd

… bug

… not using any seed, now they're flagged)

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.

Comments suppressed due to low confidence (1)

.play_minio.json:1

This file deletion removes hardcoded MinIO credentials from the repository. While this is the correct action to take, please ensure that: 1) these credentials have been rotated/invalidated since they were previously committed to version control, and 2) any systems that relied on this file have been updated with a more secure credential management approach (e.g., environment variables, secrets management system).

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

Copilot · 2025-12-17T15:00:55Z

Makefile

 run_conda: clone_yamls
-	@for seed in 2 54 546 744 1443; do \
+	mkdir -p results
+	@for seed in $(SEEDS); do \
 		echo "Running conda benchmark with seed $$seed..."; \
 		cp $(CLUSTERING_DIR)/Clustering_conda.yml $(CLUSTERING_DIR)/Clustering_conda_tmp.yml; \
-		sed -i "s/--seed, [0-9]\+/--seed, $$seed/" $(CLUSTERING_DIR)/Clustering_conda_tmp.yml; \
-		for i in 1 2 3; do \
-			echo "  Run $$i for seed $$seed..."; \
-			${OB_CMD} -b $(CLUSTERING_DIR)/Clustering_conda_tmp.yml; \
-			cp $(CLUSTERING_DIR)/Clustering_conda_tmp.yml out; \
-			mv out out_conda_seed_$$seed\_run_$$i; \
+		sed -i "s/--seed\",[[:space:]]*[0-9]\+/--seed\", $$seed/" $(CLUSTERING_DIR)/Clustering_conda_tmp.yml; \
+		for i in $(RUNS); do \
+			echo "  Run $$i for seed $$seed and run $$i."; \
+                        echo "DEST: results/out_conda_seed_$$seed\_run_$$i" ;\
+			${OB_CMD} -b $(CLUSTERING_DIR)/Clustering_conda_tmp.yml --out-dir results/out_conda_seed_$$seed\_run_$$i; \
+			cp $(CLUSTERING_DIR)/Clustering_conda_tmp.yml results/out_conda_seed_$$seed\_run_$$i/; \
 		done; \
-	rm $(CLUSTERING_DIR)/Clustering_conda_tmp.yml; \
 	done


The temporary YAML files created in each run_* target (Clustering_conda_tmp.yml, Clustering_oras_tmp.yml, Clustering_envmodules_tmp.yml) are never cleaned up. These should be removed after all runs complete to avoid leaving temporary files in the repository. Consider adding cleanup commands at the end of each target loop.

Copilot · 2025-12-17T15:00:55Z

analyze_results_izaskun.Rmd

+adj Rand index only
+
+```{r}
+# ilter to adjusted_rand_score only


Spelling error in comment: "ilter" should be "filter" (missing the leading 'f').

Copilot · 2025-12-17T15:00:56Z

parse_results.py

+    out_match = re.match(
+        r"out_(?P<backend>[a-zA-Z0-9]+)_seed_(?P<seed>\d+)_run_(?P<run>\d+)",
+        parts[1]
+    )


The function assumes the directory name will always be at parts[1], which could cause an IndexError if the path has fewer than 2 parts. Consider adding a bounds check before accessing parts[1] or handling the IndexError exception.

Copilot · 2025-12-17T15:00:56Z

parse_results.py

+            variant_name = record["method_full"]
+            if config_dir.name not in variant_name:
+                record["method_full"] = f"{variant_name}_{config_dir.name}"


The function might encounter a KeyError when accessing variant["method_full"] at line 240 if the parse_result_path function returns a variant dictionary that doesn't contain this key. This could occur if the parse_result_path function returns an empty results list or if the method_full key is not set in all code paths. Consider using variant.get("method_full", "") to safely handle missing keys.

analyze_results_izaskun.Rmd

Copilot · 2025-12-17T15:00:57Z

parse_results.py


-Pattern: out-{backend}-{rep}/data/clustbench/dataset_generator-{generator}_dataset_name-{name}/clustering/{method}
+Pattern:
+out-{backend}_seed-{seed}_run-{run}/data/clustbench/dataset_generator-{generator}_dataset_name-{name}/clustering/{method}


The pattern in the docstring at line 6 uses hyphens (out-{backend}_seed-{seed}run-{run}), but the regex pattern at line 33 expects underscores (out{backend}seed{seed}run{run}). The docstring should be corrected to match the actual implementation pattern with underscores throughout.

Copilot · 2025-12-17T15:00:57Z

analyze_results_izaskun.Rmd

+
+# Consistency checks
+
+Are there cputimes that are redudant/incosistent for repeated runs?


Spelling error: "incosistent" should be "inconsistent".

Copilot · 2025-12-17T15:00:58Z

analyze_results_izaskun.Rmd

+
+# Consistency checks
+
+Are there cputimes that are redudant/incosistent for repeated runs?


Spelling error: "redudant" should be "redundant".

parse_results.py

Makefile

imallona added 3 commits December 11, 2025 11:45

Jsonize results

466eece

Generate tabular aggregated results, long format

a8e617d

Add descriptive plots

cd8559d

imallona requested a review from Copilot December 11, 2025 16:44

Copilot started reviewing on behalf of imallona December 11, 2025 16:45 View session

Copilot AI reviewed Dec 11, 2025

View reviewed changes

imallona added 6 commits December 12, 2025 14:23

Update plots

5a6b8f5

Add plots re: seed and run sensitivity, extend benchmark

e2ffa5a

Fix parse_results.py to accept quotes in ks; add extra plots, includi…

04e4fd9

…ng Bland Altmans ks vs true k

Fixes in methods granularity in jsonizer, associated changes in R

7ddfb79

Fix early termination

229b823

Add plots, caching, disable self-contained HTML

71197a0

imallona requested a review from Copilot December 15, 2025 13:55

Copilot started reviewing on behalf of imallona December 15, 2025 14:02 View session

Copilot AI reviewed Dec 15, 2025

View reviewed changes

imallona added 2 commits December 15, 2025 15:42

Organize a bit, fix typos

ef63a58

Add censored cpu_time plot, add consistency checks, document possible…

1995ab6

… bug

imallona force-pushed the imallona_dev branch from d1c6ae3 to eb0cca7 Compare December 17, 2025 12:19

Fix seeds in Makefile, before they were ran all with seed 2

de9fc91

imallona force-pushed the imallona_dev branch from eb0cca7 to de9fc91 Compare December 17, 2025 12:46

imallona added 2 commits December 17, 2025 15:15

Several bugfixes (we're running extra rounds with 'seeds' for methods…

79a365e

… not using any seed, now they're flagged)

Deduplicate repeated runs and plot averages instead

564b208

imallona requested a review from Copilot December 17, 2025 14:55

Copilot started reviewing on behalf of imallona December 17, 2025 14:55 View session

Copilot AI reviewed Dec 17, 2025

View reviewed changes

Document, parallelize load

58ad0aa

imallona marked this pull request as ready for review December 18, 2025 08:58

imallona added 3 commits December 18, 2025 13:16

Simplify, bugfix

d7e9f6a

Casting and NA filtering

206e6f2

Add cpu_time scatters by backend; Mark

9af1b77

	geom_jitter(width = 0.2, alpha = 0.4, size = 1, fill = "black") +
	geom_jitter(width = 0.2, alpha = 0.4, size = 1, color = "black") +

-                record["method"] = m.group(1)
+                method_from_dir = m.group(1)
+                if "method" in record and record["method"] != method_from_dir:
+                    record["method_from_dir"] = method_from_dir
+                # else: do not overwrite record["method"]

		if config_dir.name not in variant_name:
		record["method_full"] = f"{variant_name}_{config_dir.name}"


		# Consistency checks

		Are there cputimes that are redudant/incosistent for repeated runs?

Update reporting / figures #10

Are you sure you want to change the base?

Update reporting / figures #10

Uh oh!

Conversation

imallona commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

imallona commented Dec 11, 2025 •

edited

Loading