Skip to content
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

Adding grid_file_path to recipe metadata for uploads #268

Merged
merged 2 commits into from
Jul 1, 2024

Conversation

rugeli
Copy link
Collaborator

@rugeli rugeli commented Jun 24, 2024

Problem

What is the problem this work solves, including
closes #266

Solution

What I/we did to solve this problem

  • relocated function: removed get_only_recipe_metadata from recipe_loader.py, and added get_recipe_metadata in upload.py
  • modified the function to explicitly check for and extract the grid_file_path if it exists in recipe data

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Steps to Verify:

  1. Upload a recipe that includes the grid_file_path to firebase. Run upload -r cellpack/tests/recipes/v2/test_url_load.json
  2. Verify that the recipe contains all the expected fields.

Note: At this stage of development, we'll need to delete any existing recipe in firebase before re-uploading a recipe with the same name. Discussions are ongoing about enabling options to overwrite or update existing recipes

Copy link

github-actions bot commented Jun 24, 2024

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

Packing image

Distance analysis

Expected minimum distance: 50.00
Actual minimum distance: 50.01

Ingredient key Pairwise distance distribution
ext_A Distance distribution ext_A

@rugeli rugeli requested review from mogres and meganrm June 24, 2024 23:22
Copy link
Collaborator

@mogres mogres left a comment

Choose a reason for hiding this comment

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

The changes look good! I was able to upload and run the example recipe from firebase.

"bounding_box": loader.recipe_data["bounding_box"],
"composition": {},
}
if "grid_file_path" in loader.recipe_data:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: should we check for all keys in loader.recipe_data and add them to the recipe_meta_data (except composition)? This way we won't have to do a special check for grid_file_path or any new keys we add to the recipe down the line.

Copy link
Collaborator Author

@rugeli rugeli Jun 26, 2024

Choose a reason for hiding this comment

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

Thank you for asking! I was thinking the same approach: iterating over recipe data and recording every key except for objects, representations, etc., to make the uploading more programmatic. However I went with specified keys(for now) so we get to fully control which keys are displayed and needed in the metadata. Helping to prevent errors during data handling.

And you're right, if there will be more new top-layer keys, we should think about a more dynamic way to manage it.

@rugeli rugeli merged commit d0b05e2 into main Jul 1, 2024
7 checks passed
@rugeli rugeli deleted the feature/add-grid-file-path branch July 1, 2024 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upload grid file path to firebase on upload
3 participants