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

[itops] split convmat in allocation and a convmat_inplace builder fun… #13

Merged
merged 3 commits into from
Feb 4, 2025

Conversation

HugoStrand
Copy link
Contributor

Dear Jason,

I have been struggling with imtime_ops.convmat for systems having a large number of orbitals. The main challenge has been that of getting control of allocations, so that the large convolution matrix is only allocated once.

This pull requests splits the current imtime_ops.convmat in two functions, retaining the API of the convmat and adding a convmat_inplace method that builds the matrix in-place using a passed matrix_view. This way it is possible for the library user to take control over the allocation.

I am currently using it on 1.1.x (and this is a pull request to the 1.1.x. branch), together with Triqs 1.3.x (that depends on cppdlr 1.1.x).

Cheers, Hugo

@jasonkaye
Copy link
Collaborator

Hi Hugo,

Thanks, this is very useful. Sorry for the long delay on this. Could you please do the following?

  • Make your pull request to merge into the main (development) branch, not the 1.1.x branch. Then we can eventually include it in a new release. We don't want to go back and modify the 1.1.x branch. If this requires making a new pull request, please do so and refer to this pull request, which we will close.
  • Add a bit of documentation explaining to users why there are separate convmat and convmat_inplace functions, and why they would want to use one or the other.

In addition, I am curious why you are pre-building the matrix of convolution using convmat, rather than just calling convolve on its own to use the fast, on-the-fly convolution algorithm (I'm sure there's a good reason since you are the one who invented that algorithm, but I'm just curious what it is).

Thanks,

Jason

@jasonkaye jasonkaye self-assigned this Jan 29, 2025
@HugoStrand HugoStrand changed the base branch from 1.1.x to main February 4, 2025 10:45
@HugoStrand
Copy link
Contributor Author

Dear Jason,

Thank you for the feeback! I have changed the target branch of the pull request to cppdlr/main and added a note in the documentation of convmat_inplace on the reason for using inplace functions, please see

https://github.com/flatironinstitute/cppdlr/pull/13/files#diff-2fcdda76424e17940339d93d6601f926c90527cd0642af6374f1fef968b8c1d6R500

or below for an excerpt.

Could you please have a look and let me know what you think?

Cheers, Hugo

Note in doc of convmat_inplace:

/*    
* \note This function builds the matrix of convolution, in place, in the
* provided matrix `fconv`. This makes it possible to control the memory
* allocation externally. If this is not a concern, we advice using the
* `covmat(...)` function instead (of `convmat_inplace(...)`).
*/

@jasonkaye jasonkaye merged commit e18a569 into flatironinstitute:main Feb 4, 2025
2 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