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

Vectorize BadELF #606

Merged
merged 51 commits into from
Feb 12, 2024
Merged

Vectorize BadELF #606

merged 51 commits into from
Feb 12, 2024

Conversation

SWeav02
Copy link
Contributor

@SWeav02 SWeav02 commented Jan 23, 2024

This pull request aims to vectorize the various components of the BadELF algorithm. This can result in speedups of a couple orders of magnitude. This pull request will include the following:

  • Update partitioning plane selection
  • Use numpy or dask array methods for assigning voxels not split by planes
  • Use numpy or dask array methods for calculating fraction of voxels split by planes
  • Clean up comments
  • Test

@SWeav02
Copy link
Contributor Author

SWeav02 commented Jan 26, 2024

@jacksund It's looking like there are more bugs with the new code than I was expecting so I probably won't be able to get it done by tomorrow. You can go ahead and release v0.16.0 with the old code and close issue #546. I'll let you know when this is looking better and is ready for a new release.

@jacksund
Copy link
Owner

Sounds good. We can do another release whenever you're ready.

I'll be making the v0.16.0 release tomorrow some time (I got caught up in some other things today)

@jacksund
Copy link
Owner

also FYI -- the CI failing isn't your fault. Black received an update last night that I will fix tomorrow morning: #607

@SWeav02
Copy link
Contributor Author

SWeav02 commented Feb 6, 2024

@jacksund As a heads up this is almost ready. I'll be testing some things a bit more tomorrow. Assuming all goes well I'll mark it ready for review.

With the new numpy based badelf, I haven't implemented a core selection. This only effects the Dask arrays that are used for larger voxel grids. I can probably add the parameter back in if it appears to be important.
@jacksund
Copy link
Owner

jacksund commented Feb 7, 2024

I'm not not sure what you just did with the commits haha. Normally you "rebase" (there is/was a button for it on this PR), but you somehow managed to duplicate all of the commits on main instead. If the branch is all out of wack, I may just move your badelf changes into a new MR.

Let me know if/when you'd need help with this

@SWeav02
Copy link
Contributor Author

SWeav02 commented Feb 7, 2024

Lol I tried rebasing in gitkraken and probably messed something up somewhere in the process. I'm making one more commit to suppress warnings within context and then I'm done on my end.

I've also run into a few issues after the rebase with running workflows. Not sure if those are caused by the improper rebase or something else, but I'll send info about them once I'm done with working out the warnings.

@SWeav02
Copy link
Contributor Author

SWeav02 commented Feb 7, 2024

@jacksund Everything seems to be running fine outside of the error I'm running into in issue #620. Once that's fixed I'll mess around with a few different structures to make sure I didn't miss any unexpected errors and then mark this ready for review.

@jacksund jacksund marked this pull request as ready for review February 7, 2024 20:08
@jacksund jacksund marked this pull request as draft February 7, 2024 20:08
@SWeav02
Copy link
Contributor Author

SWeav02 commented Feb 7, 2024

Everything is working smoothly on my end. I think the tests are still failing because of black. @jacksund I can look into it tomorrow if you don't have a quick fix.

@SWeav02 SWeav02 marked this pull request as ready for review February 7, 2024 22:23
@jacksund
Copy link
Owner

@SWeav02 you have a branch called update_electride_search too -- did you want that part of the MR?

@@ -56,6 +56,7 @@ def add(app_name: str):
if app_name == "badelf":
settings.add_apps_and_update(
[
"simmate.apps.configs.BadelfConfig",
Copy link
Owner

Choose a reason for hiding this comment

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

thanks for catching my typo 😄

@SWeav02
Copy link
Contributor Author

SWeav02 commented Feb 12, 2024

@jacksund No I'm currently messing around with stuff on that branch and it won't be ready for a bit. It's mostly testing some things for a new project.

@jacksund
Copy link
Owner

sounds good 👍 I'll merge this once the CI finishes

@jacksund jacksund merged commit 5b6d9d2 into jacksund:main Feb 12, 2024
9 checks passed
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.

2 participants