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

Discussion: API changes after code rewrite #40

Open
bast opened this issue Sep 8, 2020 · 3 comments
Open

Discussion: API changes after code rewrite #40

bast opened this issue Sep 8, 2020 · 3 comments
Labels

Comments

@bast
Copy link
Member

bast commented Sep 8, 2020

To address #39 but also other smaller issues that I was unhappy with I am in the process of rewriting the code from ground up. Rewrite is basically done and will go to master soon (few small things remain). It will be written in Rust with Rust and Python bindings, available on Crates and PyPI. This fits better my current ecosystem. I think Python bindings are getting more and more important. I might reintroduce C and Fortran bindings but for the moment want to optimize for easy pip-installability, speed, and code reliability.

For numgrid 2.0 I would like to also update the API since some of the constraints of current API would then be gone. And for this I would love to hear what other people think.

Currently numgrid generates one center at a time and the motivation for this was:

  • Since in the radial grid we do not know a priori how many points we get, this was a way to find out the number of points without waiting too much
  • I wanted to give the caller the flexibility to parallelize over these however they wanted
  • For big grids I thought this was better, but current code scales badly for the partitioning part anyway

Main question/consideration:

  • Should I keep evaluating one center at a time? (internal detail is that this makes screening the Becke partitioning a bit more difficult since I don't know the basis sets on other centers)
  • Or rather let the code compute the grid for the entire molecule/system? But also allowing to compute only subsets? (this would make screening a bit easier)

My feeling is that people want the whole grid in one shot most of the time and the API should optimize for that. But is that so?

@manassharma07
Copy link

First of all thanks for following up on it and trying to resolve the issue.

Secondly, I must admit that I'm not much knowledgeable in the field of grids generation. But anyways here are my two cents.

As you mentioned. that the advantage of doing one centre at a time was the flexibility over calling and parallelisation. For example, I was able to parallelise the grid generation over the different centres fairly easily.

However, if the code computes the grid in one shot, you say it would allow for easier screening (and probably more efficiency?). This along with the feature to compute subsets would also offer the flexibility that the existing implementation has.

So, in conclusion if I understand correctly, then I guess computing the grid in one shot along with the capability to do it only for subset would be great.

I just have one question though. In such a case, how will the parallelisation come into the picture?
Or is it the case that a single core computation of the grids would be fast enough with new optimisations to not need it?
Or would the users have to parallelise over subsets? In which case I'm afraid the less efficient screening may again make it slow?

@bast
Copy link
Member Author

bast commented Sep 8, 2020

My idea was to parallelize within the library using all available cores on a node (shared memory parallelization). In addition one could parallelize outside using MPI or something else but hopefully it won't be necessary.

If I go for the "compute all molecule" with possibility of specifying subsets, then screening can be done I think rather efficiently so I am also tending towards changing the API for that. Another motivation: somebody who wants to use it should not have to write too many lines, just send in coordinates and charges, specify basis set, and few seconds later max the grid should be there.

@manassharma07
Copy link

manassharma07 commented Sep 8, 2020

Parallelisation within the library sounds great. All the more easier for the user.

Considering that majority of the use cases would require the whole system grid itself, it would make the library much more direct to use. Sounds really awesome. Looking forward to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants