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

Use multiple gradients on a single ingredient #270

Merged
merged 7 commits into from
Jul 8, 2024

Conversation

mogres
Copy link
Collaborator

@mogres mogres commented Jun 26, 2024

Problem

Closes #262

Solution

Added support to allow multiple gradients on a single ingredient at the ingredient level.

Type of change

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

Change summary:

  • Allow the gradient attribute of an ingredient to be a List instance with multiple gradients.
  • While creating an ingredient, check if it requires multiple gradients. If yes, add an attribute combined_weight that specifies the probabilities of the grid point to be picked to place the ingredient.
  • Refactor functions in the Gradient class to be static methods.
  • Add example recipe and edit config.

Steps to Verify:

Pack a recipe with an X and Y gradient combined:

pack -r cellpack/tests/recipes/v2/test_combined_gradient.json -c cellpack/tests/packing-configs/test_grid_plot_config.json

Screenshots:

Weight map:

combined_gradient

Simularium screenshot

image

@mogres mogres requested review from rugeli and meganrm June 26, 2024 21:30
@mogres mogres self-assigned this Jun 26, 2024
@mogres mogres added the enhancement New feature or request label Jun 26, 2024
Copy link

github-actions bot commented Jun 26, 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

Copy link
Collaborator

@rugeli rugeli left a comment

Choose a reason for hiding this comment

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

I was able to run and see the combined gradient results, very cool! Just one minor suggestion, everything else look good

Comment on lines 831 to 836
if not isinstance(ingr.gradient, list):
self.center = center = self.env.gradients[
ingr.gradient
].mode_settings.get("center", center)
else:
self.center = center
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe simply reverse if and else statements for readability?

Suggested change
if not isinstance(ingr.gradient, list):
self.center = center = self.env.gradients[
ingr.gradient
].mode_settings.get("center", center)
else:
self.center = center
if isinstance(ingr.gradient, list):
self.center = center
else:
self.center = self.env.gradients[
ingr.gradient
].mode_settings.get("center", center)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks Ruge! This reminded me of another edge case of a single gradient in a list so I added that in as well here.

for gradient_name, gradient in self.gradients.items()
if gradient_name in ingr.gradient
]
combined_weight = Gradient.get_combined_gradient_weight(
Copy link
Member

Choose a reason for hiding this comment

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

can you push the logic above into a generic Gradient.pick_point_from_weight that figures out wether it needs to combine weights and returns the ptInd

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call! I added a static method Gradient.pick_point_for_ingredient here

@mogres mogres requested review from meganrm and rugeli July 2, 2024 19:03
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.73%. Comparing base (843dfec) to head (7a880a9).
Report is 11 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #270      +/-   ##
==========================================
- Coverage   98.75%   98.73%   -0.03%     
==========================================
  Files          19       18       -1     
  Lines         563      553      -10     
==========================================
- Hits          556      546      -10     
  Misses          7        7              

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

@mogres mogres merged commit 12276f5 into main Jul 8, 2024
7 checks passed
@mogres mogres deleted the feature/multiple-gradients branch July 8, 2024 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Combine multiple gradients on a single ingredient
4 participants