dgasmith / gau2grid

Fast computation of a gaussian and its derivative on a grid.
https://gau2grid.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
29 stars 17 forks source link

More generic C interface #41

Closed wavefunction91 closed 5 years ago

wavefunction91 commented 5 years ago

Note: this is not a bug, just a suggestion for an improvement.

Would it be possible to generate a more generic C interface viz the specification style for Cartesian points into the drivers? Currently, the specification dictates that the Cartesian points be specified as 3 unit-stride arrays of length "N". Specifically what I'm looking for is one of the following

  1. A specification which allows a single unit stride array of length 3*N which contains the cartesian points contiguously

  2. A more flexible specification which allows the user to specify the stride of the X,Y,Z arrays, e.g. the X array could be length N with stride 3.

dgasmith commented 5 years ago

I could be up for overhauling the interface. As a note memory access is already one of the limiting factors in gau2grid, we would need to look into how to unpack a xyzxyz... style array quickly. I believe we could still do it in a vectorized manner.

A bit torn on the two interfaces: 1) Would be less flexible, but we could optimize for it. 2) Would be more flexible and more canonical C. We could also optimize for it when stride==1 is current style and stride==3 is the xyzxyz style.

This would coincide with a major version bump where we would also allow any spherical or cartesian order to be returned.

@cryos this may help you as well, any thoughts here?

wavefunction91 commented 5 years ago

Yes, memory access will definitely be the sticking point (as it always is...), especially because xyz loads will not be along cache lines. Are you currently packing into contiguous aligned aux memory?

I agree on the interfaces as well. Though one would typically have one vs the other, it would likely be good if complete flexibility in stride specification was allowed

On Tue, Feb 19, 2019, 15:57 Daniel Smith notifications@github.com wrote:

I could be up for overhauling the interface. As a note memory access is already one of the limiting factors in gau2grid, we would need to look into how to unpack a xyzxyz... style array quickly. I believe we could still do it in a vectorized manner.

A bit torn on the two interfaces:

  1. Would be less flexible, but we could optimize for it.
  2. Would be more flexible and more canonical C. We could also optimize for it when stride==1 is current style and stride==3 is the xyzxyz style.

This would coincide with a major version bump where we would also allow any spherical or cartesian order to be returned.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dgasmith/gau2grid/issues/41#issuecomment-465361310, or mute the thread https://github.com/notifications/unsubscribe-auth/AF68vSzf6WgYIvv0X1St1CU6wlAG0zbLks5vPI81gaJpZM4bDZ3U .

dgasmith commented 5 years ago

Yes, we pull from the provided xyz pointer into a small aligned memory cache like so:

double* PRAGMA_RESTRICT cache_data = (double*)ALIGNED_MALLOC(64, 224 * sizeof(double));

The function signatures start here and you would need to edit the code that pulls from the input buffers here.

cryos commented 5 years ago

@dgasmith I would love an array of length 3n with x, y, z ordering - this is how we store stuff, and would enable me not to repack. This is a smaller part of the integration effort though, and so is more of a convenience/avoiding some copies. 2 seems like a way of making you do the rearrange for me :slightly_smiling_face:, my x, y, z ordered array I will give you three times with a stride of three and a start offset by one each time.

dgasmith commented 5 years ago

Since we are breaking the API for a v2, it seems appropriate to do this.

If we agree to simplify the input to a xyz* input where stride==3 (or greater than 3) will indicate the an array of xyz xyz ... or a stride=4 perhaps xyzw xyzw... for those which wish to bake in weights. Or a stride==1 would indicate a xx... yy... zz... style input.

If this works for both @cryos and @wavefunction91 my next PR will be to add this.

wavefunction91 commented 5 years ago

@dgasmith that sounds perfect to me.

On Sat, Jul 13, 2019, 18:17 Daniel Smith notifications@github.com wrote:

Since we are breaking the API for a v2, it seems appropriate to do this.

If we agree to simplify the input to a xyz* input where stride==3 (or greater than 3) will indicate the an array of xyz xyz ... or a stride=4 perhaps xyzw xyzw... for those which wish to bake in weights. Or a stride==1 would indicate a xx... yy... zz... style input.

If this works for both @cryos https://github.com/cryos and @wavefunction91 https://github.com/wavefunction91 my next PR will be to add this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dgasmith/gau2grid/issues/41?email_source=notifications&email_token=ABPLZPJYUVI3UGH5YWTUSPLP7J47PA5CNFSM4GYNTXKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZ34GNY#issuecomment-511165239, or mute the thread https://github.com/notifications/unsubscribe-auth/ABPLZPJYN34WUTBGORL5PFTP7J47PANCNFSM4GYNTXKA .

cryos commented 5 years ago

Me too, I would really appreciate this change @dgasmith!

dgasmith commented 5 years ago

PR #49 is up! I think this may be the last thing preventing a v2.0 release with the new API. It would be great if someone could test the new code in production.

wavefunction91 commented 5 years ago

@dgasmith, I'll put it into NWX and get back.

On Mon, Jul 15, 2019, 20:02 Daniel Smith notifications@github.com wrote:

PR #49 https://github.com/dgasmith/gau2grid/pull/49 is up! I think this may be the last thing preventing a v2.0 release with the new API. It would be great if someone could test the new code in production.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dgasmith/gau2grid/issues/41?email_source=notifications&email_token=ABPLZPNPSFZBTXT4DGOG2LDP7UFVBA5CNFSM4GYNTXKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZ7JOEY#issuecomment-511612691, or mute the thread https://github.com/notifications/unsubscribe-auth/ABPLZPMHURW76B45N7F5NSTP7UFVBANCNFSM4GYNTXKA .

dgasmith commented 5 years ago

Awesome! I will drop it into Psi4 as well.

wavefunction91 commented 5 years ago

@dgasmith This works in NWX

dgasmith commented 5 years ago

Great! I will try this out in Psi4 this weekend and if all goes well create a v2 release.

dgasmith commented 5 years ago

Closing this, v2 is now out.