-
Notifications
You must be signed in to change notification settings - Fork 17
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
Prevent splitting commas in SMARTS expressions #120
base: master
Are you sure you want to change the base?
Conversation
Hi, cool, thanks for the contribution, I enabled the test run. |
Sure, one test case added |
Hi, I can confirm that things compile, the test fails with current master and succeeds with your patch. |
I was wondering the same when I saw this... |
I found the problem when I used the "FilterSmartsInclusionList" parameter to pre-filter the candidates. the value I used for this parameter was
So the right split would be Any other comma split would result in incorrect SMARTS expressions, causing errors in the constructor function of Also, I suspected the "FilterSmartsExclusionList" parameter would face the same issue. So, I doubt splitting all commas would be a feature. |
OK so part of the problem is that (as you mentioned) commas are valid separators within SMARTS expressions contained within square brackets (as are semi-colons), but the expression you provided seems to require a comma and space combination as a separator for examples that are not in square brackets (which is two separators, not one). Is the space mandatory in the fix? Are you able to provide a candidate list/query parameters that you used together with this SMARTS expression, to test this for ourselves? |
No, the space is not a part of the separator. The original code and mine both trim the substring. Here is a parameter file I used
|
The SMARTS expressions with commas in the parameters like "FilterSmartsInclusionList" will be wrongly split right now, since all the commas are split in the current code.
As far as I know, the comma in SMARTS expressions only appears in paired square brackets.
Therefore, I added a new function to prevent splitting the commas in SMARTS expressions based on the above fact.
Hope it can be useful.