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

Should be trying variable numbers of layers? #53

Open
bannanc opened this issue Oct 26, 2018 · 1 comment
Open

Should be trying variable numbers of layers? #53

bannanc opened this issue Oct 26, 2018 · 1 comment
Labels
long term Caitlin did not have time to address question Needs to be explored further

Comments

@bannanc
Copy link
Member

bannanc commented Oct 26, 2018

Right now, well in PR #48, SMIRKSifier automatically choses the number of layers, but it just increases the number of layers with each try. However, This doesn't completely make sense since the first SMIRKS in the list shouldn't need extra layers, and so on. However, there might be places where the clusters that are hard to distinguish aren't right next to each other in the list. This brute force check slows things down significantly.

Maybe there is something we could do to make a most similar based on the ClusterGraphs and then order and determine the number of layers based on that.

@bannanc bannanc added the question Needs to be explored further label Oct 26, 2018
@bannanc
Copy link
Member Author

bannanc commented Jul 2, 2019

I think the general answer to this is YES, but I will not implement it now.

For example, we at least know that extra layers are never necessary for the first pattern in a list since it could end up being the most generic. At minimum, I suggest switching to only adding layers to the necessary patterns. That is if you had 5 clusters you would start with all clusters getting a SMIRKS with layers=0 then if that doesn't separate the clusters you would use layers=0 for the first cluster and layers=1 for clusters 2-5. Then on the next round you would use layers=0 for the first cluster, layers=1 for the second, and layers=2 for clusters 3-5.

The ordered list layers is the minimal solution, however, its possible there are only a few SMIRKS which really need an extra layer. I still can't figure out a way to add these without either doing a brute force check all patterns. An alternative solution is to explore removing layers in the Reducer you could have a remove_last_layer option that just gets rid of all the outer most layer in that pattern.

@bannanc bannanc added the long term Caitlin did not have time to address label Jul 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
long term Caitlin did not have time to address question Needs to be explored further
Projects
None yet
Development

No branches or pull requests

1 participant