-
Notifications
You must be signed in to change notification settings - Fork 7
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
[BRAINSTORM] refactor to use MOF
objects as inputs
#421
Labels
Comments
MOF
objects as inputs MOF
objects as inputs
also, testing the implementation now, another benefit seems to be that I no longer need to think about the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I'm thinking of switching to a different design pattern to make the implementation of new featurizers, especially based on fragments, graphs, and building blocks, easier.
I started introducing more and more conditionals on the input types for featurizers, be it graphs/molecules/structures which make the code really complicated -- especially if we also want to cache as much as possible.
All of this would be much easier if the inputs for the featurizers would always be
MOF
objects. I already started implementingMOF
objects formofchecker
andmoffragmentor
and they typically have (memoized) methods for computing graph/fragments/hashes/...If we were to use them, the object would simply "cache" the graph and the featurizer would no longer need to worry about it. Also, the featurizers could simply get the object attributes it needs (and the caller would not need to worry about what to provide, i.e. this logic is only implemented in one place).
In the long run, this might also make the implementation of the datasets easier as, again, the logic is only on the structure object and not partially re-implemented on the datasets.
Advantages
mofchecker
andmoffragmentor
. Could, perhaps, be factored out in its very thin own library to just provide the API (to be seen)MOFBBs
objectslru_cache
anymoreRACs
would no longer need to have their own nearest-neighbor search thingDisadvantages
matminer
, i.e., we would basically always extract the structure attribute and then pass this to pymatgenMultipleFeaturizer
with mixed types. Probably, the recommended usage pattern would be to use a MultipleFeaturizer around BUFeaturizers if needed and the BU featurizer always only operates on one type which we could then easily extract and pass to the featurizer. Caching of the fragments is no problem as it is handled by the MOF objectUsage example
Implementation idea
The featurizers would have the following signatures
In the last case, for the
BUFeaturizer
, the featurizer can inspect if it needs to get the molecules/structures/graphs from the fragments and then call the_featurize
method of the featurizer passed in the constructor.The
MOFMultipleFeaturizer
should probably always loop over structures forfeaturize_many
.Also, if I'd do something like
featurizer = MOFMultipleFeaturizer([BUFeaturizer(LSOP()), BUFeaturizer(Dimensionality()), RACS()])
it should work without problems as all featurizers always acceptMOF
objects.The text was updated successfully, but these errors were encountered: