validate recipes during load by rugeli · Pull Request #402 · mesoscope/cellpack

rugeli · 2025-10-02T20:15:17Z

Problem

closes #401

Solution

move validation to RecipeLoader._read() after data migration
- fixed validation error when packing v1 recipes
- consolidated validation to one spot that works for all recipes (v1/v2/firebase)
Simplify validate.py to use RecipeLoader for all recipe
improved data migration tests for orientBiasRotRange

Type of change

Bug fix (non-breaking change which fixes an issue)
Refactor (non-breaking change which adds functionality)

Steps to Verify:

run validate [ANY-VALID-RECIPE-PATH]
- v1 validate examples/recipes/v1/one_sphere.json
- v2 validate examples/recipes/v2/pcna_surface_gradient.json
- firebase validate firebase:recipes/gradients_v_default -d
run pack -r [ANY-VALID-RECIPE-PATH]

github-actions · 2025-10-02T20:16:29Z

Packing analysis report

Analysis for packing results located at cellpack/tests/outputs/test_spheres/spheresSST

Ingredient name	Encapsulating radius	Average number packed
ext_A	25	236.0

Packing image

Distance analysis

Expected minimum distance: 50.00
Actual minimum distance: 50.01

Ingredient key	Pairwise distance distribution
ext_A

ascibisz

I really like the code change moving the validation to recipe_loader! I tested it out and it works great.

I don't fully understand the context of the changes in migrate_v1_to_v2, but the code looks good and is very readable. But just wanted to call out that blindspot I have, I don't know much about migrate_v1_to_v2 and if you have concerns about that code change, I would suggest tagging another reviewer who has more context.

rugeli · 2025-10-06T22:32:51Z

cellpack/autopack/loaders/migrate_v1_to_v2.py

+    else:
+        range_min = -pi
+        range_max = pi
+


fixed a bug in the conversion function to prevent invalid ranges where min>max (e.g. [6, pi])

rugeli · 2025-10-06T22:34:16Z

cellpack/tests/test_recipe_version_migration.py

                    },
                    "B": {
-                        "orient_bias_range": [6, pi],
+                        "orient_bias_range": [-pi, 6],


updated test data for orient_bias_range accordingly

Not sure why the test_single_spheres recipe has these values for the rotation bias range.

A thought: all angles should be converted to be within the range [-pi, pi] before any subsequent comparisons. [6,12] doesn't really make any geometric sense.

This should also make the convert_rotation_range easier to implement. Something like

def constrain_angle(angle): return (angle + np.pi) % (2 * np.pi) - np.pi

could help

mogres

Love the recipe validation during read time! I have some comments about the orientation bias range conversion, but the validation updates look good to me!

mogres · 2025-10-06T23:42:24Z

cellpack/autopack/loaders/migrate_v1_to_v2.py

-        if "orientBiasRotRangeMax" in old_ingredient
-        else pi
-    )
+    has_min = "orientBiasRotRangeMin" in old_ingredient


This logic flow is a bit confusing. Are there any example recipes where the min range is greater than the max range which requires doing this correction?

mogres · 2025-10-06T23:43:08Z

cellpack/autopack/loaders/recipe_loader.py


 import cellpack.autopack as autopack
+
+log = logging.getLogger(__name__)


This line can go after the imports

mogres · 2025-10-06T23:43:51Z

cellpack/autopack/loaders/recipe_loader.py

+        # validate recipe after migration to v2.1 format but before transforming to class instances
+        try:
+            RecipeValidator.validate_recipe(recipe_data)
+            log.info("Recipe validation passed")


Maybe this should be a debug statement instead of info?

mogres · 2025-10-06T23:45:39Z

cellpack/bin/validate.py

-
-            RecipeValidator.validate_recipe(raw_recipe_data)
-            log.info(f"Local recipe {raw_recipe_data['name']} is valid!")
+        use_docker = recipe_path.startswith("firebase:")


Is this always true? What about users packing firebase recipes locally? Maybe the variable name in RecipeLoader could be updated to cover these use cases?

good point! I agree the name use_docker is a bit confusing here, and we can totally replace the hardcoded "firebase" with DATABSE_IDS. I'll make a overall refinement!

mogres · 2025-10-06T23:46:06Z

cellpack/bin/validate.py

+        use_docker = recipe_path.startswith("firebase:")
+        loader = RecipeLoader(recipe_path, use_docker=use_docker)
+        recipe_data = loader.recipe_data
+        log.info(f"Recipe {recipe_data['name']} is valid!")


Should probably be a debug instead of info.

mogres · 2025-10-07T00:28:02Z

cellpack/tests/test_recipe_version_migration.py

                    },
                    "B": {
-                        "orient_bias_range": [6, pi],
+                        "orient_bias_range": [-pi, 6],


Not sure why the test_single_spheres recipe has these values for the rotation bias range.

A thought: all angles should be converted to be within the range [-pi, pi] before any subsequent comparisons. [6,12] doesn't really make any geometric sense.

This should also make the convert_rotation_range easier to implement. Something like

def constrain_angle(angle): return (angle + np.pi) % (2 * np.pi) - np.pi

could help

* fix v1-v2 orient bias range migration tests * Update cellpack/autopack/loaders/migrate_v1_to_v2.py Co-authored-by: Saurabh Mogre <saurabh.mogre@alleninstitute.org> * Update cellpack/autopack/loaders/migrate_v1_to_v2.py Co-authored-by: Saurabh Mogre <saurabh.mogre@alleninstitute.org> * Update cellpack/tests/test_recipe_version_migration.py Co-authored-by: Saurabh Mogre <saurabh.mogre@alleninstitute.org> * Update cellpack/tests/test_recipe_version_migration.py Co-authored-by: Saurabh Mogre <saurabh.mogre@alleninstitute.org> * Update cellpack/tests/test_recipe_version_migration.py Co-authored-by: Saurabh Mogre <saurabh.mogre@alleninstitute.org> * Update cellpack/tests/test_recipe_version_migration.py Co-authored-by: Saurabh Mogre <saurabh.mogre@alleninstitute.org> * Update cellpack/tests/test_recipe_version_migration.py Co-authored-by: Saurabh Mogre <saurabh.mogre@alleninstitute.org> * Update cellpack/tests/test_recipe_version_migration.py Co-authored-by: Saurabh Mogre <saurabh.mogre@alleninstitute.org> * Update cellpack/tests/test_recipe_version_migration.py Co-authored-by: Saurabh Mogre <saurabh.mogre@alleninstitute.org> * Update cellpack/tests/test_recipe_version_migration.py Co-authored-by: Saurabh Mogre <saurabh.mogre@alleninstitute.org> * Update cellpack/tests/test_recipe_version_migration.py Co-authored-by: Saurabh Mogre <saurabh.mogre@alleninstitute.org> * Update cellpack/tests/test_recipe_version_migration.py Co-authored-by: Saurabh Mogre <saurabh.mogre@alleninstitute.org> * Fix indent * update validation * remove range swap * fix compartment test --------- Co-authored-by: Saurabh Mogre <saurabh.mogre@alleninstitute.org>

rugeli added 2 commits October 2, 2025 12:46

consolidate validation into recipe loader

a66e850

improve migration tests

a9cda3b

rugeli force-pushed the feature/consolidate-validation branch from d894a7c to a9cda3b Compare October 2, 2025 22:55

rugeli requested a review from ascibisz October 2, 2025 23:28

ascibisz approved these changes Oct 6, 2025

View reviewed changes

rugeli commented Oct 6, 2025

View reviewed changes

rugeli requested a review from mogres October 6, 2025 22:34

mogres approved these changes Oct 7, 2025

View reviewed changes

rugeli mentioned this pull request Oct 7, 2025

fix v1-v2 orient bias range migration tests #409

Merged

rugeli and others added 4 commits October 8, 2025 09:19

reordered logger

5fed4a7

switch info logs to debug

fcc1f49

refactor validate remote recipes

b9be65a

rugeli merged commit 567eecc into main Oct 10, 2025
7 checks passed

rugeli deleted the feature/consolidate-validation branch October 10, 2025 21:10


		import cellpack.autopack as autopack

		log = logging.getLogger(__name__)

Conversation

rugeli commented Oct 2, 2025

Problem

Solution

Type of change

Steps to Verify:

Uh oh!

github-actions bot commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Packing analysis report

Analysis for packing results located at cellpack/tests/outputs/test_spheres/spheresSST

Packing image

Distance analysis

Uh oh!

ascibisz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mogres left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Oct 2, 2025 •

edited

Loading