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

remove pandas3d #226

Merged
merged 2 commits into from
Feb 12, 2024
Merged

remove pandas3d #226

merged 2 commits into from
Feb 12, 2024

Conversation

meganrm
Copy link
Member

@meganrm meganrm commented Jan 29, 2024

Problem

pandas3d slow to install and we're not currently using it. Closes #219

Solution

removed it

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Steps to Verify:

  1. all existing recipes should work as expected

Copy link

github-actions bot commented Jan 29, 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

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e8c834e) 98.63% compared to head (0b7a859) 98.63%.

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

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #226   +/-   ##
=======================================
  Coverage   98.63%   98.63%           
=======================================
  Files          18       18           
  Lines         511      511           
=======================================
  Hits          504      504           
  Misses          7        7           

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

@rugeli
Copy link
Collaborator

rugeli commented Feb 2, 2024

Looks good to me! Here are my observations:

  1. Local v2 recipes tested with either jitter or spheresSST as the place_method were successfully packed:
  • gradients.json
  • nested.json
  • one_sphere.json
  • partner_packing_fv2.1.json
  • peroxisomes.json
  • sphere_tree.json
  1. V1 recipes were working as expected as well.
  2. We should delete the unused recipe examples/recipes/v2/peroxisomes_gradient.json. Although I believe Saurabh had previously removed it, it appears to still be present in main
  3. A thought unrelated to this PR: are we using or planning to use place_method == spring?

@meganrm meganrm requested review from mogres, kvegesna and rugeli February 2, 2024 22:56
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.

Great update! Nice to get rid of unused functionality.

@@ -94,16 +94,14 @@ Display packing in realtime (slow)

### place_method

_Optional `enum`_. One of `"jitter"`, `"spheresSST"`, `"pandaBullet"`.. Default: `"jitter"`.
_Optional `enum`_. One of `"jitter"`, `"spheresSST"`.. Default: `"jitter"`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the current default spheresSST?

@@ -2701,324 +2648,26 @@ def setupOctree(
self.grid.getRadius(), helper=helper
) # Octree((0,0,0),self.grid.getRadius()) #0,0,0 or center of grid?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This function used to take so long to run! Good to have it removed

@mogres
Copy link
Collaborator

mogres commented Feb 11, 2024

We can merge this one if ready, @meganrm

@meganrm meganrm merged commit 72ecf15 into main Feb 12, 2024
7 checks passed
@meganrm meganrm deleted the fix/remove-panda3d branch February 12, 2024 03:03
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.

Consider removing panda3d dependency?
4 participants