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

Pass cosmetic attrs through to Potential objects #917

Closed
SimonBoothroyd opened this issue Mar 3, 2024 · 3 comments
Closed

Pass cosmetic attrs through to Potential objects #917

SimonBoothroyd opened this issue Mar 3, 2024 · 3 comments

Comments

@SimonBoothroyd
Copy link
Contributor

Description

Currently cosmetic attributes on force field parameters do not propagate through to a Potential objects parameters. Currently we need to re-look them up from the original force field.

It would be great if these could be passed through.

@SimonBoothroyd
Copy link
Contributor Author

My current hacky workaround:

def to_interchange(ff, top):
    inter = openff.interchange.Interchange.from_smirnoff(ff, top)

    for col_k, col_v in inter.collections.items():
        for pot_k, pot_v in col_v.potentials.items():
            if " " not in pot_k.id:
                handler = ff[pot_k.associated_handler]
                param = handler[pot_k.id]
            else:
                smirks, lp, match = pot_k.id.split(" ")
                handler = ff["VirtualSites"]
                (param,) = handler.get_parameter(
                    {"smirks": smirks, "name": lp, "match": match}
                )

            if not hasattr(param, "_cosmetic_attribs"):
                continue

            for attr in param._cosmetic_attribs:
                pot_v.parameters[attr] = getattr(param, f"_{attr}")

    return inter

@mattwthompson
Copy link
Member

Seems fair, I don't use cosmetic attributes so this never occurred to me.

I figure the last loop in that block of code should do the trick, and having it turned on by default makes sense

@mattwthompson
Copy link
Member

@SimonBoothroyd Looking at this more closely now - does it make sense to have it on the Potential or PotentialKey object? Right now Potential.parameters really only stores the stuff that describes the physics of the interaction, whereas i.e. stuff like parameterize_eval shoved in for ForceBalance's accounting seem like things that better live on pointers to those objects. I'm not sure if such a physics vs. bookkeeping distinction holds for other use cases.

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

No branches or pull requests

2 participants