Modularize the evaluation fo the Fourier domain contributions #48
Conversation
Added tests for grid mesh Moved example dependences to pp.ml Some polish to mesh-demo
Also modified PMEPotential as an example of application
There was a problem hiding this comment.
Thanks for contribution! Overall I'm fine with factoring out the k-space filtering, I think. I'm a bit worried about the increasing amounts of indirection and things being stored as references everywhere (LRFilter acquiring some pointer to a potential, for example) or suddenly _smearing being an attribute. But these are minor issues that can be corrected easily.
Co-authored-by: Philip Loche <philip.loche@posteo.de>
There was a problem hiding this comment.
Some new example files are good, but this draft overall still looks quite rough. The three main things that need to be changed before this gets merged in my opinion are:
- When it comes to the provided examples, this looks more like someone's personal playground, rather than a library that was designed with users in mind. The existing example notebooks can be a good starting point, and I provided more specific comments to those.
- For now, the benefits of going back to extra classes for the Fourier part is not as clear to me (and you promised us that you would convince us). Instead of adding examples on each sub-module, it would be more beneficial to see how this benefits somebody using the
pmecalculatordirectly, which is how we mostly intend to interact with this code. - The new classes need unit tests. Some can be resurrected from the time when I had written them and adapted a little.
Also, please try to use slightly more descriptive commit messages. While some are really nice (and I appreciate those a lot!), something as short as "lint", "fix filename", "update examples" etc. would already a lot better than some of the remaining ones. This is especially important now to understand what has been addressed.
Co-authored-by: Philip Loche <philip.loche@posteo.de>
|
Merged main, and I'd say this is ready to be given one last pass and merged. The examples go quite crazy in demonstrating integration, autograd and torchscripting, which i think will be very useful for "power users" wanting to use this in models. |
There was a problem hiding this comment.
I rebased your branch.
During the rebase, I found a couple of things we should discuss:
- If we pass
smearingas a parameter that needs to be backpropagatable, we should define it as a tensor in the calculators as well. I haven’t done this yet because the rebase was already quite painful, and the new design has made the data flow unclear to me. smearing, which is a parameter that can change during runtime, was set by you at the initialization level. I think the previous approach— obtaining the smearing from theatomic_smearingand passing the smearing parameter when calling the potential methods—was much clearer. This also avoids resetting thesmearingproperty in each step.- Another general comment: Personally, I don’t define default values in private functions. This makes the internal code more explicit and easier to read, especially when you haven't touched the code in a while.
* renamed from_dist_?r * eliminated kspace_scaling parameter * cleaned up a few bits and pieces
Fixes #16
This introduces a KSpaceFilter class that streamlines the definition and application of a convolution kernel in reciprocal space, as well as a reorganization of the potentials to use play along well with this format.
Furthermore, added two more examples demonstrating the use of these building blocks to make new models, that double up as tests for rather complicated interactions with torch.
📚 Documentation preview 📚: https://torch-pme--48.org.readthedocs.build/en/48/