Skip to content

Feat/week4 density api#233

Open
kir943 wants to merge 8 commits intotheochem:masterfrom
kir943:feat/week4-density-api
Open

Feat/week4 density api#233
kir943 wants to merge 8 commits intotheochem:masterfrom
kir943:feat/week4-density-api

Conversation

@kir943
Copy link

@kir943 kir943 commented Feb 24, 2026

Hi @msricher,

Summary

This PR implements the Week 4 milestone of the Winter of Code project by adding the density API layer built on top of the arbitrary-order overlap engine.

Specifically:

• Added new module gbasis.integrals.density
• Implemented compute_intracule and compute_extracule functions
• Integrated these APIs with the existing arbitrary_order_overlap sparse tensor engine
• Added dedicated unit tests (test_integrals_density.py)

Validation

• All existing tests pass successfully (304 passed)
• New density API tests added and passing

Scope

This PR introduces the density API interface and does not modify existing overlap engine functionality.
Please let me know if any refinements are needed. I will proceed with Week 5 tasks next.

Thank you!

Copy link
Collaborator

@msricher msricher left a comment

Choose a reason for hiding this comment

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

I cannot find anything wrong here. I think this is in line with what sort of interface we want. @marco-2023 are you more familiar with this?

norm_bound = 1.0

for shell in shells:
coeff_bound *= np.max(np.abs(shell.coeffs))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd check the performance in practice, but sometimes it's faster to use something that just queries the original array A, like max(A.min(), A.max(), key=abs), instead of actually computing np.abs(A).

Copy link
Author

Choose a reason for hiding this comment

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

Thank you @msricher for the suggestion and for pointing out the performance aspect.

I agree that avoiding the intermediate allocation from np.abs can be a cleaner and potentially more efficient approach. This could be updated to:

coeff_bound *= max(abs(shell.coeffs.min()), abs(shell.coeffs.max()))

This avoids allocating a temporary array, which may improve performance and reduce memory usage, especially for larger coefficient arrays. On the other hand, the current implementation using np.max(np.abs(...)) is slightly more explicit and consistent with common NumPy idioms.

Please let me know if you would like me to incorporate this change in the current PR.

@msricher
Copy link
Collaborator

msricher commented Feb 24, 2026 via email

@kir943
Copy link
Author

kir943 commented Feb 28, 2026

Hi @msricher,

Thank you for the clarification!

Since the Week 4 work is complete and all checks are passing, I have opened a separate PR for the Week 5 tasks, which includes tests, benchmarks, and real-basis validation for the density API.

Please let me know if you would like any changes or improvements. I’ll be happy to update it accordingly.

Thank you!

@msricher
Copy link
Collaborator

msricher commented Mar 2, 2026

@kir943 I think the code is good enough to merge. Could you run your code through the black formatter with arguments black -l 100 -t py39 <FILES>, and then open a new PR with the entire WoC project weeks 1-5, with organized commits (i.e. just do a git rebase and merge/organize stuff if necessary, or maybe it's already nice enough...)? Then I can make sure one of the main maintainers reviews the whole thing, and then we'll merge it all together.

@kir943
Copy link
Author

kir943 commented Mar 2, 2026

Hi @msricher,

Thank you for your guidance.

I have now run the code through the black formatter as requested and opened a new pull request that combines the complete WoC implementation for Weeks 1–5 into a single branch with organized commits:

All checks are passing successfully.Please let me know if any further changes or refinements are needed. I’d be happy to update it accordingly.
Thank you!

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