From: SourceForge.net <no...@so...> - 2011-10-05 14:33:35
|
Patches item #3419026, was opened at 2011-10-05 10:33 Message generated for change (Tracker Item Submitted) made by rajarshi You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3419026&group_id=20024 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: cdk-1.4.x Group: Needs Review Status: Open Resolution: None Priority: 7 Private: No Submitted By: Rajarshi Guha (rajarshi) Assigned to: Egon Willighagen (egonw) Summary: Allow SmilesGenerator to output aromatic SMILES and also mak Initial Comment: The two patches modify the SmilesGenerator to 1) allow the generation of aromatic fragments 2) Not modify (mostly) the input molecule. 1) is achieved by changing the meaning of the useAromaticityFlag to be more obvious: if set to FALSE, the SG will perceieve aromaticity. If set to TRUE, it will use whatever flags are specified in the input molecule. So if you have benzene, but did not perceive aromaticity you will get a non-aromatic SMILES. Also made ring perception and ato typing independent of this flag 2) is achieved by using a clone of the input, but only in createSMILESWithoutCheckForMultipleMolecules. This means that if you call createSmiles, the inputmolecule is changed - but onlhy n the VISITED flag. To make this cleaner, we either make createSMILESWithoutCheckForMultipleMolecules private and clone the molecule in createSmiles; ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3419026&group_id=20024 |
From: SourceForge.net <no...@so...> - 2011-10-05 14:34:20
|
Patches item #3419026, was opened at 2011-10-05 10:33 Message generated for change (Comment added) made by rajarshi You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3419026&group_id=20024 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: cdk-1.4.x Group: Needs Review Status: Open Resolution: None Priority: 7 Private: No Submitted By: Rajarshi Guha (rajarshi) Assigned to: Egon Willighagen (egonw) Summary: Allow SmilesGenerator to output aromatic SMILES and also mak Initial Comment: The two patches modify the SmilesGenerator to 1) allow the generation of aromatic fragments 2) Not modify (mostly) the input molecule. 1) is achieved by changing the meaning of the useAromaticityFlag to be more obvious: if set to FALSE, the SG will perceieve aromaticity. If set to TRUE, it will use whatever flags are specified in the input molecule. So if you have benzene, but did not perceive aromaticity you will get a non-aromatic SMILES. Also made ring perception and ato typing independent of this flag 2) is achieved by using a clone of the input, but only in createSMILESWithoutCheckForMultipleMolecules. This means that if you call createSmiles, the inputmolecule is changed - but onlhy n the VISITED flag. To make this cleaner, we either make createSMILESWithoutCheckForMultipleMolecules private and clone the molecule in createSmiles; ---------------------------------------------------------------------- >Comment By: Rajarshi Guha (rajarshi) Date: 2011-10-05 10:34 Message: Also added appropriate unit tests and modifed some other unit tests to take this behavior into account. Also updated main Javadocs ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3419026&group_id=20024 |
From: SourceForge.net <no...@so...> - 2011-10-05 17:40:01
|
Patches item #3419026, was opened at 2011-10-05 16:33 Message generated for change (Comment added) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3419026&group_id=20024 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: cdk-1.4.x Group: Needs Review Status: Open Resolution: None Priority: 7 Private: No Submitted By: Rajarshi Guha (rajarshi) Assigned to: Egon Willighagen (egonw) Summary: Allow SmilesGenerator to output aromatic SMILES and also mak Initial Comment: The two patches modify the SmilesGenerator to 1) allow the generation of aromatic fragments 2) Not modify (mostly) the input molecule. 1) is achieved by changing the meaning of the useAromaticityFlag to be more obvious: if set to FALSE, the SG will perceieve aromaticity. If set to TRUE, it will use whatever flags are specified in the input molecule. So if you have benzene, but did not perceive aromaticity you will get a non-aromatic SMILES. Also made ring perception and ato typing independent of this flag 2) is achieved by using a clone of the input, but only in createSMILESWithoutCheckForMultipleMolecules. This means that if you call createSmiles, the inputmolecule is changed - but onlhy n the VISITED flag. To make this cleaner, we either make createSMILESWithoutCheckForMultipleMolecules private and clone the molecule in createSmiles; ---------------------------------------------------------------------- >Comment By: Egon Willighagen (egonw) Date: 2011-10-05 19:40 Message: Rajarshi, I am against the solution for 1, because: 1. it changes the current implementation (OK in master, but I do not like that in cdk-1.4.x) 2. it no longer is possible to not have aromatic SMILES if your input was 'wrong' Yet, I agree that the useAromaticityFlag was ambigously named. I always interpreted this boolean as the flag to indicate that the generation should use the aromaticity information to create 'aromatic SMILES', but understand your interpretation fully too. I suggest two flags, on the basis of my second argument: - useAromaticityFlag → indicating that the IChemObject flag CDKConstants.IS_AROMATIC should be used, rather than doing perception itself - outputAromaticSmiles → indicating if aromatic or non-aromatic SMILES should be generated. This changes the API too. Both options must be reviewed by a second reviewer, but up front, the above is my suggested revision. ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2011-10-05 16:34 Message: Also added appropriate unit tests and modifed some other unit tests to take this behavior into account. Also updated main Javadocs ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3419026&group_id=20024 |
From: SourceForge.net <no...@so...> - 2011-10-08 03:03:43
|
Patches item #3419026, was opened at 2011-10-05 10:33 Message generated for change (Comment added) made by rajarshi You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3419026&group_id=20024 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: cdk-1.4.x Group: Needs Review Status: Open Resolution: None Priority: 7 Private: No Submitted By: Rajarshi Guha (rajarshi) Assigned to: Egon Willighagen (egonw) Summary: Allow SmilesGenerator to output aromatic SMILES and also mak Initial Comment: The two patches modify the SmilesGenerator to 1) allow the generation of aromatic fragments 2) Not modify (mostly) the input molecule. 1) is achieved by changing the meaning of the useAromaticityFlag to be more obvious: if set to FALSE, the SG will perceieve aromaticity. If set to TRUE, it will use whatever flags are specified in the input molecule. So if you have benzene, but did not perceive aromaticity you will get a non-aromatic SMILES. Also made ring perception and ato typing independent of this flag 2) is achieved by using a clone of the input, but only in createSMILESWithoutCheckForMultipleMolecules. This means that if you call createSmiles, the inputmolecule is changed - but onlhy n the VISITED flag. To make this cleaner, we either make createSMILESWithoutCheckForMultipleMolecules private and clone the molecule in createSmiles; ---------------------------------------------------------------------- >Comment By: Rajarshi Guha (rajarshi) Date: 2011-10-07 23:03 Message: Hmm, I see your point in 1). However., I'd argue that the proposed match makes the 1.4 API "sensible". Not sure what you mean in point 2. I don't see a problem with adding an extra flag, but I'd rather see that the current useAromaticityFlag be fixed. I that means shifting to master that's fine with me ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-10-05 13:40 Message: Rajarshi, I am against the solution for 1, because: 1. it changes the current implementation (OK in master, but I do not like that in cdk-1.4.x) 2. it no longer is possible to not have aromatic SMILES if your input was 'wrong' Yet, I agree that the useAromaticityFlag was ambigously named. I always interpreted this boolean as the flag to indicate that the generation should use the aromaticity information to create 'aromatic SMILES', but understand your interpretation fully too. I suggest two flags, on the basis of my second argument: - useAromaticityFlag → indicating that the IChemObject flag CDKConstants.IS_AROMATIC should be used, rather than doing perception itself - outputAromaticSmiles → indicating if aromatic or non-aromatic SMILES should be generated. This changes the API too. Both options must be reviewed by a second reviewer, but up front, the above is my suggested revision. ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2011-10-05 10:34 Message: Also added appropriate unit tests and modifed some other unit tests to take this behavior into account. Also updated main Javadocs ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3419026&group_id=20024 |
From: SourceForge.net <no...@so...> - 2011-10-08 06:37:01
|
Patches item #3419026, was opened at 2011-10-05 16:33 Message generated for change (Comment added) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3419026&group_id=20024 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: cdk-1.4.x Group: Needs Review Status: Open Resolution: None Priority: 7 Private: No Submitted By: Rajarshi Guha (rajarshi) Assigned to: Egon Willighagen (egonw) Summary: Allow SmilesGenerator to output aromatic SMILES and also mak Initial Comment: The two patches modify the SmilesGenerator to 1) allow the generation of aromatic fragments 2) Not modify (mostly) the input molecule. 1) is achieved by changing the meaning of the useAromaticityFlag to be more obvious: if set to FALSE, the SG will perceieve aromaticity. If set to TRUE, it will use whatever flags are specified in the input molecule. So if you have benzene, but did not perceive aromaticity you will get a non-aromatic SMILES. Also made ring perception and ato typing independent of this flag 2) is achieved by using a clone of the input, but only in createSMILESWithoutCheckForMultipleMolecules. This means that if you call createSmiles, the inputmolecule is changed - but onlhy n the VISITED flag. To make this cleaner, we either make createSMILESWithoutCheckForMultipleMolecules private and clone the molecule in createSmiles; ---------------------------------------------------------------------- >Comment By: Egon Willighagen (egonw) Date: 2011-10-08 08:37 Message: Sorry for not being clear. If you add a second flag, then I'm fine with changing the API to make it sensible for 1.4.x. About point 2... if I understand your one-param solution correctly, it was no longer possible to have no perceived aromaticity at all anymore, because both TRUE and FALSE would perceive arom, just in different ways. This too would be fixed by having two params. ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2011-10-08 05:03 Message: Hmm, I see your point in 1). However., I'd argue that the proposed match makes the 1.4 API "sensible". Not sure what you mean in point 2. I don't see a problem with adding an extra flag, but I'd rather see that the current useAromaticityFlag be fixed. I that means shifting to master that's fine with me ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-10-05 19:40 Message: Rajarshi, I am against the solution for 1, because: 1. it changes the current implementation (OK in master, but I do not like that in cdk-1.4.x) 2. it no longer is possible to not have aromatic SMILES if your input was 'wrong' Yet, I agree that the useAromaticityFlag was ambigously named. I always interpreted this boolean as the flag to indicate that the generation should use the aromaticity information to create 'aromatic SMILES', but understand your interpretation fully too. I suggest two flags, on the basis of my second argument: - useAromaticityFlag → indicating that the IChemObject flag CDKConstants.IS_AROMATIC should be used, rather than doing perception itself - outputAromaticSmiles → indicating if aromatic or non-aromatic SMILES should be generated. This changes the API too. Both options must be reviewed by a second reviewer, but up front, the above is my suggested revision. ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2011-10-05 16:34 Message: Also added appropriate unit tests and modifed some other unit tests to take this behavior into account. Also updated main Javadocs ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3419026&group_id=20024 |
From: SourceForge.net <no...@so...> - 2011-10-19 07:29:36
|
Patches item #3419026, was opened at 2011-10-05 16:33 Message generated for change (Settings changed) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3419026&group_id=20024 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: cdk-1.4.x >Group: Needs Revision Status: Open Resolution: None Priority: 7 Private: No Submitted By: Rajarshi Guha (rajarshi) Assigned to: Egon Willighagen (egonw) Summary: Allow SmilesGenerator to output aromatic SMILES and also mak Initial Comment: The two patches modify the SmilesGenerator to 1) allow the generation of aromatic fragments 2) Not modify (mostly) the input molecule. 1) is achieved by changing the meaning of the useAromaticityFlag to be more obvious: if set to FALSE, the SG will perceieve aromaticity. If set to TRUE, it will use whatever flags are specified in the input molecule. So if you have benzene, but did not perceive aromaticity you will get a non-aromatic SMILES. Also made ring perception and ato typing independent of this flag 2) is achieved by using a clone of the input, but only in createSMILESWithoutCheckForMultipleMolecules. This means that if you call createSmiles, the inputmolecule is changed - but onlhy n the VISITED flag. To make this cleaner, we either make createSMILESWithoutCheckForMultipleMolecules private and clone the molecule in createSmiles; ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-10-08 08:37 Message: Sorry for not being clear. If you add a second flag, then I'm fine with changing the API to make it sensible for 1.4.x. About point 2... if I understand your one-param solution correctly, it was no longer possible to have no perceived aromaticity at all anymore, because both TRUE and FALSE would perceive arom, just in different ways. This too would be fixed by having two params. ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2011-10-08 05:03 Message: Hmm, I see your point in 1). However., I'd argue that the proposed match makes the 1.4 API "sensible". Not sure what you mean in point 2. I don't see a problem with adding an extra flag, but I'd rather see that the current useAromaticityFlag be fixed. I that means shifting to master that's fine with me ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-10-05 19:40 Message: Rajarshi, I am against the solution for 1, because: 1. it changes the current implementation (OK in master, but I do not like that in cdk-1.4.x) 2. it no longer is possible to not have aromatic SMILES if your input was 'wrong' Yet, I agree that the useAromaticityFlag was ambigously named. I always interpreted this boolean as the flag to indicate that the generation should use the aromaticity information to create 'aromatic SMILES', but understand your interpretation fully too. I suggest two flags, on the basis of my second argument: - useAromaticityFlag → indicating that the IChemObject flag CDKConstants.IS_AROMATIC should be used, rather than doing perception itself - outputAromaticSmiles → indicating if aromatic or non-aromatic SMILES should be generated. This changes the API too. Both options must be reviewed by a second reviewer, but up front, the above is my suggested revision. ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2011-10-05 16:34 Message: Also added appropriate unit tests and modifed some other unit tests to take this behavior into account. Also updated main Javadocs ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3419026&group_id=20024 |
From: SourceForge.net <no...@so...> - 2012-01-25 05:43:56
|
Patches item #3419026, was opened at 2011-10-05 07:33 Message generated for change (Settings changed) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3419026&group_id=20024 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: cdk-1.4.x Group: Needs Revision Status: Open Resolution: None Priority: 7 Private: No Submitted By: Rajarshi Guha (rajarshi) >Assigned to: Rajarshi Guha (rajarshi) Summary: Allow SmilesGenerator to output aromatic SMILES and also mak Initial Comment: The two patches modify the SmilesGenerator to 1) allow the generation of aromatic fragments 2) Not modify (mostly) the input molecule. 1) is achieved by changing the meaning of the useAromaticityFlag to be more obvious: if set to FALSE, the SG will perceieve aromaticity. If set to TRUE, it will use whatever flags are specified in the input molecule. So if you have benzene, but did not perceive aromaticity you will get a non-aromatic SMILES. Also made ring perception and ato typing independent of this flag 2) is achieved by using a clone of the input, but only in createSMILESWithoutCheckForMultipleMolecules. This means that if you call createSmiles, the inputmolecule is changed - but onlhy n the VISITED flag. To make this cleaner, we either make createSMILESWithoutCheckForMultipleMolecules private and clone the molecule in createSmiles; ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-10-07 23:37 Message: Sorry for not being clear. If you add a second flag, then I'm fine with changing the API to make it sensible for 1.4.x. About point 2... if I understand your one-param solution correctly, it was no longer possible to have no perceived aromaticity at all anymore, because both TRUE and FALSE would perceive arom, just in different ways. This too would be fixed by having two params. ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2011-10-07 20:03 Message: Hmm, I see your point in 1). However., I'd argue that the proposed match makes the 1.4 API "sensible". Not sure what you mean in point 2. I don't see a problem with adding an extra flag, but I'd rather see that the current useAromaticityFlag be fixed. I that means shifting to master that's fine with me ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-10-05 10:40 Message: Rajarshi, I am against the solution for 1, because: 1. it changes the current implementation (OK in master, but I do not like that in cdk-1.4.x) 2. it no longer is possible to not have aromatic SMILES if your input was 'wrong' Yet, I agree that the useAromaticityFlag was ambigously named. I always interpreted this boolean as the flag to indicate that the generation should use the aromaticity information to create 'aromatic SMILES', but understand your interpretation fully too. I suggest two flags, on the basis of my second argument: - useAromaticityFlag → indicating that the IChemObject flag CDKConstants.IS_AROMATIC should be used, rather than doing perception itself - outputAromaticSmiles → indicating if aromatic or non-aromatic SMILES should be generated. This changes the API too. Both options must be reviewed by a second reviewer, but up front, the above is my suggested revision. ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2011-10-05 07:34 Message: Also added appropriate unit tests and modifed some other unit tests to take this behavior into account. Also updated main Javadocs ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3419026&group_id=20024 |