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

Some clarifications and wrapup for graphgen #107

Open
mcocdawc opened this issue Jan 30, 2025 · 2 comments
Open

Some clarifications and wrapup for graphgen #107

mcocdawc opened this issue Jan 30, 2025 · 2 comments
Assignees
Labels
documentation Improvements or additions to documentation
Milestone

Comments

@mcocdawc
Copy link
Contributor

While testing and comparing the different fragmentation schemes I encountered a few suprises, which might be relevant for other users.

Currently the documentation of graphgen
reads as

[...] Generalizes the BEn fragmentation scheme to arbitrary fragment sizes using a graph theoretic heuristic. [...]

It would be a good idea to change this wording a bit, to brand "graphgen" more as an alternative fragmentation scheme, than a generalisation, since it has a 0.9 kJ / mol difference for BE2 on octane.

Furthermore it might surprise some users that BE5, or larger values such as BE20, does not produce a single fragment for a sufficiently small molecule, such as octane. If this really is intended behaviour it might be a good idea to mention it.

The cutoff=20 is too low. The result of the fragmentation should be stable under the default value. At least for BE4 on octane it should be at least 70. To have some buffer it is probably good to put the default in the range of 80-100.

@mcocdawc mcocdawc added the documentation Improvements or additions to documentation label Jan 30, 2025
@mcocdawc mcocdawc added this to the v0.1 milestone Jan 30, 2025
@ShaunWeatherly
Copy link
Contributor

ShaunWeatherly commented Jan 31, 2025

@mcocdawc I don't think I ever said that my code was a generalization of autogen, rather it is a generalization of the BEn atom-centered partitioning scheme (which exists outside of any particular code). And not replicating autogen is kind of the point - afterall, if it did nothing new then it really isn't adding anything whatsoever to the codebase.

And I still don't think the case of 1 "fragment" is really relevant to the function of a fragmentation code, but I can add it if you insist it's necessary.

Likewise I'll update the default value for cutoff as soon as I can.

@mcocdawc
Copy link
Contributor Author

I fully agree that it should not even aim to replicate the output of the specific implementation autogen.

But AFAIK, so far all (?) fragmentation algorithms¹ had the general property that for n -> infty the BE(n) fragmentation converged to the whole molecule. This is also the basis for the convergence test of plotting the energy against the fragmentation level n.

If a fragmentation algorithm does not have this property anymore, i.e. for n -> infty it does not converge to the complete unfragmented system, it might be surprising behaviour; at least for me it was. One cannot expect strictly convergence to the full system for enlarging n.
This is outweighed by other advantages of the algorithm, but it would be a good idea to document it.

To summarize: If this is intended behavior, which only you can judge, then there is no need to fix it. But a note to the user might be good.

¹ Even the ones on the Hubbard model with multiple origins per fragment.

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

No branches or pull requests

2 participants