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

Freeze arguments to pick_calculator #2695

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

zulissimeta
Copy link
Contributor

Summary of Changes

lru_cache is used to make sure that each MLP calculator is only instantiated once (great!). However, if you want to pass dictionaries through to the underlying calculator (say, config overrides to the fairchem calculator), you will get errors because dictionaries are mutable and lru_cache is unhappy. We can patch this by converting any dictionary kwargs to a frozendict.

Requirements

Note: If you are an external contributor, you will see a comment from @buildbot-princeton. This is solely for the maintainers.

@Andrew-S-Rosen
Copy link
Member

@zulissimeta: Interesting. Thank you for the PR here.

A question: what is the benefit in using frozendict as opposed to frozenset without requiring an additional dependency? For instance,

d = {"cow": "moo", "cat": "meow"}
d_frozen = frozenset(d.items()) # frozen now!
d_unfrozen = dict(d_frozen) # same as d again!

You would still need a decorator, but I haven't quite figured out yet the value-add for frozendict. If we do need frozendict though, this should be added to pyproject.toml as well.

@Andrew-S-Rosen
Copy link
Member

Ah, frozenset will not work on a nested dict like d={"cow": {"moo": 1}}.

While not particularly elegant, I am okay with the proposed approach if there's no other option since frozendict seems reasonably well maintained. Not a huge fan of having another dependency just for this, but I don't know of a better solution. Happy to merge with a pyprojoect.toml update and your approval that it's ready.

@zulissimeta
Copy link
Contributor Author

Thanks, think it should be good to go now.

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

Successfully merging this pull request may close these issues.

2 participants