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

Refactoring and Cleanup of Interface #84

Merged
merged 43 commits into from
Jun 19, 2024
Merged

Refactoring and Cleanup of Interface #84

merged 43 commits into from
Jun 19, 2024

Conversation

cortner
Copy link
Member

@cortner cortner commented Jun 3, 2024

The purpose of this PR started as an experiment moving the entire P4ML package over to using Bumper and WithAlloc and removing all usage of ObjectPools. It now includes

  • fully retiring OP in favour of Bumper
  • cleanup of the pb and pfwd interface
  • remove all hand-written pb2 and pwfd
  • cleanup of the type hierarchy
  • cleanup of allocating / non-allocating interface
  • cleanup sphericart interface and retire the P4ML 3D harmonics implementations
  • some improvements to unified testing

My sense now is that the extra cost of allocations in typical ACE models will cost a moderate % of runtime, somewhere between 30-50% typically. This is more than acceptable for prototyping. For fast final models we can then hand-write the chains to get the best possible performance.

I'm unifying and finalizing the pb/pfwd interface while I do this so I can write as much shared code as possible.

TODO

  • decide on whether we support laplacians in P4ML or only the ability to use hyper-duals. It is not clear to me what is the right choice.
  • bring the linear layer back (as discussed with Jerry)
  • make sure the sparseproduct has the functionality needed elsewhere.
  • cleanup and check the lux interface
  • confirm the sphericart wrapper serves our purpose
  • rename -> pullback! and pushforward!
  • try and test unified second pullback via ForwardDiff
  • try unified pushforward implementation via ForwardDiff
  • allocation tests for most embeddings and all tensors (can bring in more over time)
  • Extend withalloc to PooledSparseProduct

Leave for future PRs

  • complex bases seem to fail allocation tests
  • improve the unified testing functions (a lot of testing functionality is duplicated throughout the test_*** scripts
  • provide literate examples how to compose manually and how to do pfwd and pb2 through such models, compare with Lux-style implementations of the same models, advantages and disadvantages
  • literate example how to compute a laplacian
  • update benchmark code

@cortner cortner changed the title WIP: Bumper and WithAlloc WIP: Bumper+WithAlloc and Unify pb/pfwd interface Jun 4, 2024
@cortner cortner changed the title WIP: Bumper+WithAlloc and Unify pb/pfwd interface WIP: Major Cleanup Jun 6, 2024
@cortner cortner mentioned this pull request Jun 7, 2024
6 tasks
@CheukHinHoJerry
Copy link
Collaborator

Should we also remove anything related to FlexArray?

@cortner
Copy link
Member Author

cortner commented Jun 8, 2024

Yes for sure. Most of it should be gone. I just haven't reintegrated all parts of the codebase yet.

@CheukHinHoJerry
Copy link
Collaborator

CheukHinHoJerry commented Jun 15, 2024

SparseProduct looks fine elsewhere. The functionality now it provides is sufficient. I will have a final clean up now.

@cortner
Copy link
Member Author

cortner commented Jun 15, 2024

Thank you!

@cortner
Copy link
Member Author

cortner commented Jun 18, 2024

WithAlloc.jl is now registered in the general registry, which means we could start thinking about merging this PR and registering the next minor version? @CheukHinHoJerry , what is still missing?

@CheukHinHoJerry
Copy link
Collaborator

CheukHinHoJerry commented Jun 18, 2024

Sorry for late reply - I had some local changes on cleaning up the interface of SparseProduct that unifies with PooledSparseProduct. Just pushed them and now I think it is ready if the CI passes.

@CheukHinHoJerry
Copy link
Collaborator

CheukHinHoJerry commented Jun 18, 2024

CI stucks at testing SparseProduct, which probably relates to #87. That doesn't happen for me previously but it suddenly occurs for the last commit.

@cortner
Copy link
Member Author

cortner commented Jun 19, 2024

Ok, I'll take a look and see if I can fix it.

@CheukHinHoJerry
Copy link
Collaborator

CheukHinHoJerry commented Jun 19, 2024

I think I see why - I will fix it now.

@cortner
Copy link
Member Author

cortner commented Jun 19, 2024

Thank you, Jerry!

@cortner
Copy link
Member Author

cortner commented Jun 19, 2024

So maybe my last question for @DexuanZhou and @CheukHinHoJerry -- do you need laplacians HERE in P4ML or is it enough for you that you can use P4ML layers with Duals and HyperDuals?

@CheukHinHoJerry
Copy link
Collaborator

CheukHinHoJerry commented Jun 19, 2024

I think it is enough for us just to make evaluation of Duals and HyperDuals work. If there is a feature request or there are more than one projects that needs laplacian we can then consider adding that?

@cortner
Copy link
Member Author

cortner commented Jun 19, 2024

so we are actually ready to merge this? If you confirm, I'll go ahead with it :)

@CheukHinHoJerry
Copy link
Collaborator

Yes please I am happy for this to be merged.

@cortner cortner changed the title WIP: Major Cleanup Refactoring and Cleanup of Interface Jun 19, 2024
@cortner cortner merged commit 4eaff70 into main Jun 19, 2024
5 checks passed
@cortner cortner deleted the co/bumper branch June 19, 2024 19:52
@cortner
Copy link
Member Author

cortner commented Jun 19, 2024

Everyone - thank you for your feedback and help with this PR. I've now registereed this as v0.3.0 in the ACE registry (not in General yet since I'm having a problem with sphericart...)

@cortner
Copy link
Member Author

cortner commented Jun 20, 2024

This is now also registered in General

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