From: SourceForge.net <no...@so...> - 2011-06-28 13:02:32
|
Junior Jobs item #3340660, was opened at 2011-06-28 13:02 Message generated for change (Tracker Item Submitted) made by ralcantara You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=997721&aid=3340660&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: None Group: None Status: Open Resolution: None Priority: 5 Private: No Submitted By: Rafael Alcántara (ralcantara) Assigned to: Nobody/Anonymous (nobody) Summary: No formula generated when certain elements present Initial Comment: In some cases, when there is a Helium atom in a molecule (ex. http://www.ebi.ac.uk/chebi/searchId.do?chebiId=33685) the formula is not computed. This is due to that element being missing in org.openscience.cdk.tools.manipulator.MolecularFormulaManipulator.generateOrderEle* methods. The problem applies also to other elements: He, Rf, Db, Sg, Bh, Hs, Mt, Ds, Rg, Am, Cm, Bk, Cf, Es, Fm, Md, No, Lr, Uub ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=997721&aid=3340660&group_id=20024 |
From: SourceForge.net <no...@so...> - 2011-06-30 08:38:29
|
Patches item #3340660, was opened at 2011-06-28 15:02 Message generated for change (Comment added) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&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: None Group: None Status: Open Resolution: None Priority: 5 Private: No Submitted By: Rafael Alcántara (ralcantara) Assigned to: Nobody/Anonymous (nobody) Summary: No formula generated when certain elements present Initial Comment: In some cases, when there is a Helium atom in a molecule (ex. http://www.ebi.ac.uk/chebi/searchId.do?chebiId=33685) the formula is not computed. This is due to that element being missing in org.openscience.cdk.tools.manipulator.MolecularFormulaManipulator.generateOrderEle* methods. The problem applies also to other elements: He, Rf, Db, Sg, Bh, Hs, Mt, Ds, Rg, Am, Cm, Bk, Cf, Es, Fm, Md, No, Lr, Uub ---------------------------------------------------------------------- >Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:38 Message: Cool! That was actually the second unit test in my patch :) ---------------------------------------------------------------------- Comment By: Rafael Alcántara (ralcantara) Date: 2011-06-30 10:29 Message: Hi, Egon. My code is quite similar: IMolecule mol = builder.newInstance(IMolecule.class); mol.addAtom(new Atom("He")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(mol); Assert.assertEquals("He", MolecularFormulaManipulator.getString(formula)); Regards, Rafael ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 17:17 Message: Rafael, does your code look like this: IAtomContainer helium = new Molecule(); helium.addAtom(new Atom("Am")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(helium); Assert.assertNotNull(formula); Assert.assertEquals("Am", MolecularFormulaManipulator.getString(formula)); ? ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 15:04 Message: Did you also write a few unit tests? ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-29 15:00 Message: Attached are patches adding the missing elements. Missing were Helium and the tail-end of the periodic table, starting from Americium/Am. I added all elements up to and including Copernicium (#112) Whitespace was a mess (format changing _within_ lines), so I cleaned that up first in a separate patch. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&group_id=20024 |
From: SourceForge.net <no...@so...> - 2011-06-30 08:40:19
|
Patches item #3340660, was opened at 2011-06-28 15:02 Message generated for change (Comment added) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&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: 9 Private: No Submitted By: Rafael Alcántara (ralcantara) Assigned to: Nobody/Anonymous (nobody) Summary: No formula generated when certain elements present Initial Comment: In some cases, when there is a Helium atom in a molecule (ex. http://www.ebi.ac.uk/chebi/searchId.do?chebiId=33685) the formula is not computed. This is due to that element being missing in org.openscience.cdk.tools.manipulator.MolecularFormulaManipulator.generateOrderEle* methods. The problem applies also to other elements: He, Rf, Db, Sg, Bh, Hs, Mt, Ds, Rg, Am, Cm, Bk, Cf, Es, Fm, Md, No, Lr, Uub ---------------------------------------------------------------------- >Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:40 Message: Should be part of 1.4.0. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:38 Message: Cool! That was actually the second unit test in my patch :) ---------------------------------------------------------------------- Comment By: Rafael Alcántara (ralcantara) Date: 2011-06-30 10:29 Message: Hi, Egon. My code is quite similar: IMolecule mol = builder.newInstance(IMolecule.class); mol.addAtom(new Atom("He")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(mol); Assert.assertEquals("He", MolecularFormulaManipulator.getString(formula)); Regards, Rafael ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 17:17 Message: Rafael, does your code look like this: IAtomContainer helium = new Molecule(); helium.addAtom(new Atom("Am")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(helium); Assert.assertNotNull(formula); Assert.assertEquals("Am", MolecularFormulaManipulator.getString(formula)); ? ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 15:04 Message: Did you also write a few unit tests? ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-29 15:00 Message: Attached are patches adding the missing elements. Missing were Helium and the tail-end of the periodic table, starting from Americium/Am. I added all elements up to and including Copernicium (#112) Whitespace was a mess (format changing _within_ lines), so I cleaned that up first in a separate patch. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&group_id=20024 |
From: SourceForge.net <no...@so...> - 2011-06-30 09:35:30
|
Patches item #3340660, was opened at 2011-06-28 15:02 Message generated for change (Comment added) made by jkerssem You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&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: 9 Private: No Submitted By: Rafael Alcántara (ralcantara) Assigned to: Nobody/Anonymous (nobody) Summary: No formula generated when certain elements present Initial Comment: In some cases, when there is a Helium atom in a molecule (ex. http://www.ebi.ac.uk/chebi/searchId.do?chebiId=33685) the formula is not computed. This is due to that element being missing in org.openscience.cdk.tools.manipulator.MolecularFormulaManipulator.generateOrderEle* methods. The problem applies also to other elements: He, Rf, Db, Sg, Bh, Hs, Mt, Ds, Rg, Am, Cm, Bk, Cf, Es, Fm, Md, No, Lr, Uub ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-30 11:35 Message: Egon, I couldn't figure out how to unit-test a list of strings, essentially a constant. If you have suggestions on how to do this, I'll gladly write some tests. Added bonus: if new elements get accepted, we'll need to update it again. I chose to leave out the Uup, Uuh, etc. elements since you won't encounter those in 'normal' chemistry. (The same may be said for most of row 7, but ah well) ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:40 Message: Should be part of 1.4.0. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:38 Message: Cool! That was actually the second unit test in my patch :) ---------------------------------------------------------------------- Comment By: Rafael Alcántara (ralcantara) Date: 2011-06-30 10:29 Message: Hi, Egon. My code is quite similar: IMolecule mol = builder.newInstance(IMolecule.class); mol.addAtom(new Atom("He")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(mol); Assert.assertEquals("He", MolecularFormulaManipulator.getString(formula)); Regards, Rafael ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 17:17 Message: Rafael, does your code look like this: IAtomContainer helium = new Molecule(); helium.addAtom(new Atom("Am")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(helium); Assert.assertNotNull(formula); Assert.assertEquals("Am", MolecularFormulaManipulator.getString(formula)); ? ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 15:04 Message: Did you also write a few unit tests? ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-29 15:00 Message: Attached are patches adding the missing elements. Missing were Helium and the tail-end of the periodic table, starting from Americium/Am. I added all elements up to and including Copernicium (#112) Whitespace was a mess (format changing _within_ lines), so I cleaned that up first in a separate patch. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&group_id=20024 |
From: SourceForge.net <no...@so...> - 2011-07-02 16:47:27
|
Patches item #3340660, was opened at 2011-06-28 13:02 Message generated for change (Comment added) made by gilleain You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&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: Accepted Status: Open Resolution: None Priority: 9 Private: No Submitted By: Rafael Alcántara (ralcantara) Assigned to: Nobody/Anonymous (nobody) Summary: No formula generated when certain elements present Initial Comment: In some cases, when there is a Helium atom in a molecule (ex. http://www.ebi.ac.uk/chebi/searchId.do?chebiId=33685) the formula is not computed. This is due to that element being missing in org.openscience.cdk.tools.manipulator.MolecularFormulaManipulator.generateOrderEle* methods. The problem applies also to other elements: He, Rf, Db, Sg, Bh, Hs, Mt, Ds, Rg, Am, Cm, Bk, Cf, Es, Fm, Md, No, Lr, Uub ---------------------------------------------------------------------- >Comment By: gilleain maclean torrance (gilleain) Date: 2011-07-02 16:47 Message: Looks good. I approve of whitespace cleaning (and the alignment of element symbols in the array, to improve readability). Also good that this is a separate patch. One thing, though, does the CDK use tabs or spaces? I prefer spaces... ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-30 09:35 Message: Egon, I couldn't figure out how to unit-test a list of strings, essentially a constant. If you have suggestions on how to do this, I'll gladly write some tests. Added bonus: if new elements get accepted, we'll need to update it again. I chose to leave out the Uup, Uuh, etc. elements since you won't encounter those in 'normal' chemistry. (The same may be said for most of row 7, but ah well) ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 08:40 Message: Should be part of 1.4.0. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 08:38 Message: Cool! That was actually the second unit test in my patch :) ---------------------------------------------------------------------- Comment By: Rafael Alcántara (ralcantara) Date: 2011-06-30 08:29 Message: Hi, Egon. My code is quite similar: IMolecule mol = builder.newInstance(IMolecule.class); mol.addAtom(new Atom("He")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(mol); Assert.assertEquals("He", MolecularFormulaManipulator.getString(formula)); Regards, Rafael ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 15:17 Message: Rafael, does your code look like this: IAtomContainer helium = new Molecule(); helium.addAtom(new Atom("Am")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(helium); Assert.assertNotNull(formula); Assert.assertEquals("Am", MolecularFormulaManipulator.getString(formula)); ? ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 13:04 Message: Did you also write a few unit tests? ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-29 13:00 Message: Attached are patches adding the missing elements. Missing were Helium and the tail-end of the periodic table, starting from Americium/Am. I added all elements up to and including Copernicium (#112) Whitespace was a mess (format changing _within_ lines), so I cleaned that up first in a separate patch. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&group_id=20024 |
From: SourceForge.net <no...@so...> - 2011-07-02 17:13:54
|
Patches item #3340660, was opened at 2011-06-28 15:02 Message generated for change (Comment added) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&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: Accepted Status: Open Resolution: None Priority: 9 Private: No Submitted By: Rafael Alcántara (ralcantara) Assigned to: Nobody/Anonymous (nobody) Summary: No formula generated when certain elements present Initial Comment: In some cases, when there is a Helium atom in a molecule (ex. http://www.ebi.ac.uk/chebi/searchId.do?chebiId=33685) the formula is not computed. This is due to that element being missing in org.openscience.cdk.tools.manipulator.MolecularFormulaManipulator.generateOrderEle* methods. The problem applies also to other elements: He, Rf, Db, Sg, Bh, Hs, Mt, Ds, Rg, Am, Cm, Bk, Cf, Es, Fm, Md, No, Lr, Uub ---------------------------------------------------------------------- >Comment By: Egon Willighagen (egonw) Date: 2011-07-02 19:13 Message: Spaces. ---------------------------------------------------------------------- Comment By: gilleain maclean torrance (gilleain) Date: 2011-07-02 18:47 Message: Looks good. I approve of whitespace cleaning (and the alignment of element symbols in the array, to improve readability). Also good that this is a separate patch. One thing, though, does the CDK use tabs or spaces? I prefer spaces... ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-30 11:35 Message: Egon, I couldn't figure out how to unit-test a list of strings, essentially a constant. If you have suggestions on how to do this, I'll gladly write some tests. Added bonus: if new elements get accepted, we'll need to update it again. I chose to leave out the Uup, Uuh, etc. elements since you won't encounter those in 'normal' chemistry. (The same may be said for most of row 7, but ah well) ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:40 Message: Should be part of 1.4.0. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:38 Message: Cool! That was actually the second unit test in my patch :) ---------------------------------------------------------------------- Comment By: Rafael Alcántara (ralcantara) Date: 2011-06-30 10:29 Message: Hi, Egon. My code is quite similar: IMolecule mol = builder.newInstance(IMolecule.class); mol.addAtom(new Atom("He")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(mol); Assert.assertEquals("He", MolecularFormulaManipulator.getString(formula)); Regards, Rafael ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 17:17 Message: Rafael, does your code look like this: IAtomContainer helium = new Molecule(); helium.addAtom(new Atom("Am")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(helium); Assert.assertNotNull(formula); Assert.assertEquals("Am", MolecularFormulaManipulator.getString(formula)); ? ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 15:04 Message: Did you also write a few unit tests? ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-29 15:00 Message: Attached are patches adding the missing elements. Missing were Helium and the tail-end of the periodic table, starting from Americium/Am. I added all elements up to and including Copernicium (#112) Whitespace was a mess (format changing _within_ lines), so I cleaned that up first in a separate patch. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&group_id=20024 |
From: SourceForge.net <no...@so...> - 2011-07-02 18:38:35
|
Patches item #3340660, was opened at 2011-06-28 15:02 Message generated for change (Comment added) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&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: Accepted Status: Open Resolution: None Priority: 9 Private: No Submitted By: Rafael Alcántara (ralcantara) Assigned to: Nobody/Anonymous (nobody) Summary: No formula generated when certain elements present Initial Comment: In some cases, when there is a Helium atom in a molecule (ex. http://www.ebi.ac.uk/chebi/searchId.do?chebiId=33685) the formula is not computed. This is due to that element being missing in org.openscience.cdk.tools.manipulator.MolecularFormulaManipulator.generateOrderEle* methods. The problem applies also to other elements: He, Rf, Db, Sg, Bh, Hs, Mt, Ds, Rg, Am, Cm, Bk, Cf, Es, Fm, Md, No, Lr, Uub ---------------------------------------------------------------------- >Comment By: Egon Willighagen (egonw) Date: 2011-07-02 20:38 Message: Thanx for the reviewing. Applied and pushed. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-02 19:13 Message: Spaces. ---------------------------------------------------------------------- Comment By: gilleain maclean torrance (gilleain) Date: 2011-07-02 18:47 Message: Looks good. I approve of whitespace cleaning (and the alignment of element symbols in the array, to improve readability). Also good that this is a separate patch. One thing, though, does the CDK use tabs or spaces? I prefer spaces... ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-30 11:35 Message: Egon, I couldn't figure out how to unit-test a list of strings, essentially a constant. If you have suggestions on how to do this, I'll gladly write some tests. Added bonus: if new elements get accepted, we'll need to update it again. I chose to leave out the Uup, Uuh, etc. elements since you won't encounter those in 'normal' chemistry. (The same may be said for most of row 7, but ah well) ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:40 Message: Should be part of 1.4.0. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:38 Message: Cool! That was actually the second unit test in my patch :) ---------------------------------------------------------------------- Comment By: Rafael Alcántara (ralcantara) Date: 2011-06-30 10:29 Message: Hi, Egon. My code is quite similar: IMolecule mol = builder.newInstance(IMolecule.class); mol.addAtom(new Atom("He")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(mol); Assert.assertEquals("He", MolecularFormulaManipulator.getString(formula)); Regards, Rafael ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 17:17 Message: Rafael, does your code look like this: IAtomContainer helium = new Molecule(); helium.addAtom(new Atom("Am")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(helium); Assert.assertNotNull(formula); Assert.assertEquals("Am", MolecularFormulaManipulator.getString(formula)); ? ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 15:04 Message: Did you also write a few unit tests? ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-29 15:00 Message: Attached are patches adding the missing elements. Missing were Helium and the tail-end of the periodic table, starting from Americium/Am. I added all elements up to and including Copernicium (#112) Whitespace was a mess (format changing _within_ lines), so I cleaned that up first in a separate patch. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&group_id=20024 |
From: SourceForge.net <no...@so...> - 2011-07-02 18:40:21
|
Patches item #3340660, was opened at 2011-06-28 15:02 Message generated for change (Settings changed) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&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: Accepted >Status: Closed >Resolution: Fixed Priority: 9 Private: No Submitted By: Rafael Alcántara (ralcantara) Assigned to: Nobody/Anonymous (nobody) Summary: No formula generated when certain elements present Initial Comment: In some cases, when there is a Helium atom in a molecule (ex. http://www.ebi.ac.uk/chebi/searchId.do?chebiId=33685) the formula is not computed. This is due to that element being missing in org.openscience.cdk.tools.manipulator.MolecularFormulaManipulator.generateOrderEle* methods. The problem applies also to other elements: He, Rf, Db, Sg, Bh, Hs, Mt, Ds, Rg, Am, Cm, Bk, Cf, Es, Fm, Md, No, Lr, Uub ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-02 20:38 Message: Thanx for the reviewing. Applied and pushed. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-02 19:13 Message: Spaces. ---------------------------------------------------------------------- Comment By: gilleain maclean torrance (gilleain) Date: 2011-07-02 18:47 Message: Looks good. I approve of whitespace cleaning (and the alignment of element symbols in the array, to improve readability). Also good that this is a separate patch. One thing, though, does the CDK use tabs or spaces? I prefer spaces... ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-30 11:35 Message: Egon, I couldn't figure out how to unit-test a list of strings, essentially a constant. If you have suggestions on how to do this, I'll gladly write some tests. Added bonus: if new elements get accepted, we'll need to update it again. I chose to leave out the Uup, Uuh, etc. elements since you won't encounter those in 'normal' chemistry. (The same may be said for most of row 7, but ah well) ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:40 Message: Should be part of 1.4.0. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:38 Message: Cool! That was actually the second unit test in my patch :) ---------------------------------------------------------------------- Comment By: Rafael Alcántara (ralcantara) Date: 2011-06-30 10:29 Message: Hi, Egon. My code is quite similar: IMolecule mol = builder.newInstance(IMolecule.class); mol.addAtom(new Atom("He")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(mol); Assert.assertEquals("He", MolecularFormulaManipulator.getString(formula)); Regards, Rafael ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 17:17 Message: Rafael, does your code look like this: IAtomContainer helium = new Molecule(); helium.addAtom(new Atom("Am")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(helium); Assert.assertNotNull(formula); Assert.assertEquals("Am", MolecularFormulaManipulator.getString(formula)); ? ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 15:04 Message: Did you also write a few unit tests? ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-29 15:00 Message: Attached are patches adding the missing elements. Missing were Helium and the tail-end of the periodic table, starting from Americium/Am. I added all elements up to and including Copernicium (#112) Whitespace was a mess (format changing _within_ lines), so I cleaned that up first in a separate patch. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&group_id=20024 |
From: SourceForge.net <no...@so...> - 2011-07-03 07:11:31
|
Patches item #3340660, was opened at 2011-06-28 15:02 Message generated for change (Comment added) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&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: Accepted Status: Closed Resolution: Fixed Priority: 9 Private: No Submitted By: Rafael Alcántara (ralcantara) Assigned to: Nobody/Anonymous (nobody) Summary: No formula generated when certain elements present Initial Comment: In some cases, when there is a Helium atom in a molecule (ex. http://www.ebi.ac.uk/chebi/searchId.do?chebiId=33685) the formula is not computed. This is due to that element being missing in org.openscience.cdk.tools.manipulator.MolecularFormulaManipulator.generateOrderEle* methods. The problem applies also to other elements: He, Rf, Db, Sg, Bh, Hs, Mt, Ds, Rg, Am, Cm, Bk, Cf, Es, Fm, Md, No, Lr, Uub ---------------------------------------------------------------------- >Comment By: Egon Willighagen (egonw) Date: 2011-07-03 09:11 Message: BTW, the fix caused a regression: https://sourceforge.net/tracker/?func=detail&aid=3349469&group_id=20024&atid=120024 that unit test got outdated, and need updating. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-02 20:38 Message: Thanx for the reviewing. Applied and pushed. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-02 19:13 Message: Spaces. ---------------------------------------------------------------------- Comment By: gilleain maclean torrance (gilleain) Date: 2011-07-02 18:47 Message: Looks good. I approve of whitespace cleaning (and the alignment of element symbols in the array, to improve readability). Also good that this is a separate patch. One thing, though, does the CDK use tabs or spaces? I prefer spaces... ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-30 11:35 Message: Egon, I couldn't figure out how to unit-test a list of strings, essentially a constant. If you have suggestions on how to do this, I'll gladly write some tests. Added bonus: if new elements get accepted, we'll need to update it again. I chose to leave out the Uup, Uuh, etc. elements since you won't encounter those in 'normal' chemistry. (The same may be said for most of row 7, but ah well) ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:40 Message: Should be part of 1.4.0. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:38 Message: Cool! That was actually the second unit test in my patch :) ---------------------------------------------------------------------- Comment By: Rafael Alcántara (ralcantara) Date: 2011-06-30 10:29 Message: Hi, Egon. My code is quite similar: IMolecule mol = builder.newInstance(IMolecule.class); mol.addAtom(new Atom("He")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(mol); Assert.assertEquals("He", MolecularFormulaManipulator.getString(formula)); Regards, Rafael ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 17:17 Message: Rafael, does your code look like this: IAtomContainer helium = new Molecule(); helium.addAtom(new Atom("Am")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(helium); Assert.assertNotNull(formula); Assert.assertEquals("Am", MolecularFormulaManipulator.getString(formula)); ? ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 15:04 Message: Did you also write a few unit tests? ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-29 15:00 Message: Attached are patches adding the missing elements. Missing were Helium and the tail-end of the periodic table, starting from Americium/Am. I added all elements up to and including Copernicium (#112) Whitespace was a mess (format changing _within_ lines), so I cleaned that up first in a separate patch. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&group_id=20024 |
From: SourceForge.net <no...@so...> - 2011-07-04 08:31:44
|
Patches item #3340660, was opened at 2011-06-28 15:02 Message generated for change (Settings changed) made by jkerssem You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&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: Accepted >Status: Open Resolution: Fixed Priority: 9 Private: No Submitted By: Rafael Alcántara (ralcantara) Assigned to: Nobody/Anonymous (nobody) Summary: No formula generated when certain elements present Initial Comment: In some cases, when there is a Helium atom in a molecule (ex. http://www.ebi.ac.uk/chebi/searchId.do?chebiId=33685) the formula is not computed. This is due to that element being missing in org.openscience.cdk.tools.manipulator.MolecularFormulaManipulator.generateOrderEle* methods. The problem applies also to other elements: He, Rf, Db, Sg, Bh, Hs, Mt, Ds, Rg, Am, Cm, Bk, Cf, Es, Fm, Md, No, Lr, Uub ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-03 09:11 Message: BTW, the fix caused a regression: https://sourceforge.net/tracker/?func=detail&aid=3349469&group_id=20024&atid=120024 that unit test got outdated, and need updating. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-02 20:38 Message: Thanx for the reviewing. Applied and pushed. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-02 19:13 Message: Spaces. ---------------------------------------------------------------------- Comment By: gilleain maclean torrance (gilleain) Date: 2011-07-02 18:47 Message: Looks good. I approve of whitespace cleaning (and the alignment of element symbols in the array, to improve readability). Also good that this is a separate patch. One thing, though, does the CDK use tabs or spaces? I prefer spaces... ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-30 11:35 Message: Egon, I couldn't figure out how to unit-test a list of strings, essentially a constant. If you have suggestions on how to do this, I'll gladly write some tests. Added bonus: if new elements get accepted, we'll need to update it again. I chose to leave out the Uup, Uuh, etc. elements since you won't encounter those in 'normal' chemistry. (The same may be said for most of row 7, but ah well) ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:40 Message: Should be part of 1.4.0. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:38 Message: Cool! That was actually the second unit test in my patch :) ---------------------------------------------------------------------- Comment By: Rafael Alcántara (ralcantara) Date: 2011-06-30 10:29 Message: Hi, Egon. My code is quite similar: IMolecule mol = builder.newInstance(IMolecule.class); mol.addAtom(new Atom("He")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(mol); Assert.assertEquals("He", MolecularFormulaManipulator.getString(formula)); Regards, Rafael ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 17:17 Message: Rafael, does your code look like this: IAtomContainer helium = new Molecule(); helium.addAtom(new Atom("Am")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(helium); Assert.assertNotNull(formula); Assert.assertEquals("Am", MolecularFormulaManipulator.getString(formula)); ? ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 15:04 Message: Did you also write a few unit tests? ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-29 15:00 Message: Attached are patches adding the missing elements. Missing were Helium and the tail-end of the periodic table, starting from Americium/Am. I added all elements up to and including Copernicium (#112) Whitespace was a mess (format changing _within_ lines), so I cleaned that up first in a separate patch. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&group_id=20024 |
From: SourceForge.net <no...@so...> - 2011-07-04 08:34:00
|
Patches item #3340660, was opened at 2011-06-28 15:02 Message generated for change (Comment added) made by jkerssem You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&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: Accepted Status: Open Resolution: Fixed Priority: 9 Private: No Submitted By: Rafael Alcántara (ralcantara) Assigned to: Nobody/Anonymous (nobody) Summary: No formula generated when certain elements present Initial Comment: In some cases, when there is a Helium atom in a molecule (ex. http://www.ebi.ac.uk/chebi/searchId.do?chebiId=33685) the formula is not computed. This is due to that element being missing in org.openscience.cdk.tools.manipulator.MolecularFormulaManipulator.generateOrderEle* methods. The problem applies also to other elements: He, Rf, Db, Sg, Bh, Hs, Mt, Ds, Rg, Am, Cm, Bk, Cf, Es, Fm, Md, No, Lr, Uub ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-07-04 10:33 Message: re-opening to fully see it through. - updated regressed unit test (0005) - fixed whitespace for testclass first (0004) - removed meaningless test-class javadoc, wrote new oneliners where useful (0006) - changed tabs to spaces for MolecularFormulaManipulator (0007) ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-03 09:11 Message: BTW, the fix caused a regression: https://sourceforge.net/tracker/?func=detail&aid=3349469&group_id=20024&atid=120024 that unit test got outdated, and need updating. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-02 20:38 Message: Thanx for the reviewing. Applied and pushed. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-02 19:13 Message: Spaces. ---------------------------------------------------------------------- Comment By: gilleain maclean torrance (gilleain) Date: 2011-07-02 18:47 Message: Looks good. I approve of whitespace cleaning (and the alignment of element symbols in the array, to improve readability). Also good that this is a separate patch. One thing, though, does the CDK use tabs or spaces? I prefer spaces... ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-30 11:35 Message: Egon, I couldn't figure out how to unit-test a list of strings, essentially a constant. If you have suggestions on how to do this, I'll gladly write some tests. Added bonus: if new elements get accepted, we'll need to update it again. I chose to leave out the Uup, Uuh, etc. elements since you won't encounter those in 'normal' chemistry. (The same may be said for most of row 7, but ah well) ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:40 Message: Should be part of 1.4.0. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:38 Message: Cool! That was actually the second unit test in my patch :) ---------------------------------------------------------------------- Comment By: Rafael Alcántara (ralcantara) Date: 2011-06-30 10:29 Message: Hi, Egon. My code is quite similar: IMolecule mol = builder.newInstance(IMolecule.class); mol.addAtom(new Atom("He")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(mol); Assert.assertEquals("He", MolecularFormulaManipulator.getString(formula)); Regards, Rafael ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 17:17 Message: Rafael, does your code look like this: IAtomContainer helium = new Molecule(); helium.addAtom(new Atom("Am")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(helium); Assert.assertNotNull(formula); Assert.assertEquals("Am", MolecularFormulaManipulator.getString(formula)); ? ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 15:04 Message: Did you also write a few unit tests? ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-29 15:00 Message: Attached are patches adding the missing elements. Missing were Helium and the tail-end of the periodic table, starting from Americium/Am. I added all elements up to and including Copernicium (#112) Whitespace was a mess (format changing _within_ lines), so I cleaned that up first in a separate patch. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&group_id=20024 |
From: SourceForge.net <no...@so...> - 2011-07-04 08:54:24
|
Patches item #3340660, was opened at 2011-06-28 15:02 Message generated for change (Comment added) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&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: Fixed Priority: 9 Private: No Submitted By: Rafael Alcántara (ralcantara) Assigned to: Nobody/Anonymous (nobody) Summary: No formula generated when certain elements present Initial Comment: In some cases, when there is a Helium atom in a molecule (ex. http://www.ebi.ac.uk/chebi/searchId.do?chebiId=33685) the formula is not computed. This is due to that element being missing in org.openscience.cdk.tools.manipulator.MolecularFormulaManipulator.generateOrderEle* methods. The problem applies also to other elements: He, Rf, Db, Sg, Bh, Hs, Mt, Ds, Rg, Am, Cm, Bk, Cf, Es, Fm, Md, No, Lr, Uub ---------------------------------------------------------------------- >Comment By: Egon Willighagen (egonw) Date: 2011-07-04 10:54 Message: Are those updated patches, or based on CDK 1.4.0 ? ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-07-04 10:33 Message: re-opening to fully see it through. - updated regressed unit test (0005) - fixed whitespace for testclass first (0004) - removed meaningless test-class javadoc, wrote new oneliners where useful (0006) - changed tabs to spaces for MolecularFormulaManipulator (0007) ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-03 09:11 Message: BTW, the fix caused a regression: https://sourceforge.net/tracker/?func=detail&aid=3349469&group_id=20024&atid=120024 that unit test got outdated, and need updating. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-02 20:38 Message: Thanx for the reviewing. Applied and pushed. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-02 19:13 Message: Spaces. ---------------------------------------------------------------------- Comment By: gilleain maclean torrance (gilleain) Date: 2011-07-02 18:47 Message: Looks good. I approve of whitespace cleaning (and the alignment of element symbols in the array, to improve readability). Also good that this is a separate patch. One thing, though, does the CDK use tabs or spaces? I prefer spaces... ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-30 11:35 Message: Egon, I couldn't figure out how to unit-test a list of strings, essentially a constant. If you have suggestions on how to do this, I'll gladly write some tests. Added bonus: if new elements get accepted, we'll need to update it again. I chose to leave out the Uup, Uuh, etc. elements since you won't encounter those in 'normal' chemistry. (The same may be said for most of row 7, but ah well) ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:40 Message: Should be part of 1.4.0. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:38 Message: Cool! That was actually the second unit test in my patch :) ---------------------------------------------------------------------- Comment By: Rafael Alcántara (ralcantara) Date: 2011-06-30 10:29 Message: Hi, Egon. My code is quite similar: IMolecule mol = builder.newInstance(IMolecule.class); mol.addAtom(new Atom("He")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(mol); Assert.assertEquals("He", MolecularFormulaManipulator.getString(formula)); Regards, Rafael ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 17:17 Message: Rafael, does your code look like this: IAtomContainer helium = new Molecule(); helium.addAtom(new Atom("Am")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(helium); Assert.assertNotNull(formula); Assert.assertEquals("Am", MolecularFormulaManipulator.getString(formula)); ? ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 15:04 Message: Did you also write a few unit tests? ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-29 15:00 Message: Attached are patches adding the missing elements. Missing were Helium and the tail-end of the periodic table, starting from Americium/Am. I added all elements up to and including Copernicium (#112) Whitespace was a mess (format changing _within_ lines), so I cleaned that up first in a separate patch. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&group_id=20024 |
From: SourceForge.net <no...@so...> - 2011-07-04 09:06:59
|
Patches item #3340660, was opened at 2011-06-28 15:02 Message generated for change (Comment added) made by jkerssem You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&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: Fixed Priority: 9 Private: No Submitted By: Rafael Alcántara (ralcantara) Assigned to: Nobody/Anonymous (nobody) Summary: No formula generated when certain elements present Initial Comment: In some cases, when there is a Helium atom in a molecule (ex. http://www.ebi.ac.uk/chebi/searchId.do?chebiId=33685) the formula is not computed. This is due to that element being missing in org.openscience.cdk.tools.manipulator.MolecularFormulaManipulator.generateOrderEle* methods. The problem applies also to other elements: He, Rf, Db, Sg, Bh, Hs, Mt, Ds, Rg, Am, Cm, Bk, Cf, Es, Fm, Md, No, Lr, Uub ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-07-04 11:06 Message: The testclass-patches (0004-0006) are based on a clean 1.4.0; 0007 is an update to be applied after 0002, if we care about tabs-not-being-spaces ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-04 10:54 Message: Are those updated patches, or based on CDK 1.4.0 ? ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-07-04 10:33 Message: re-opening to fully see it through. - updated regressed unit test (0005) - fixed whitespace for testclass first (0004) - removed meaningless test-class javadoc, wrote new oneliners where useful (0006) - changed tabs to spaces for MolecularFormulaManipulator (0007) ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-03 09:11 Message: BTW, the fix caused a regression: https://sourceforge.net/tracker/?func=detail&aid=3349469&group_id=20024&atid=120024 that unit test got outdated, and need updating. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-02 20:38 Message: Thanx for the reviewing. Applied and pushed. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-02 19:13 Message: Spaces. ---------------------------------------------------------------------- Comment By: gilleain maclean torrance (gilleain) Date: 2011-07-02 18:47 Message: Looks good. I approve of whitespace cleaning (and the alignment of element symbols in the array, to improve readability). Also good that this is a separate patch. One thing, though, does the CDK use tabs or spaces? I prefer spaces... ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-30 11:35 Message: Egon, I couldn't figure out how to unit-test a list of strings, essentially a constant. If you have suggestions on how to do this, I'll gladly write some tests. Added bonus: if new elements get accepted, we'll need to update it again. I chose to leave out the Uup, Uuh, etc. elements since you won't encounter those in 'normal' chemistry. (The same may be said for most of row 7, but ah well) ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:40 Message: Should be part of 1.4.0. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:38 Message: Cool! That was actually the second unit test in my patch :) ---------------------------------------------------------------------- Comment By: Rafael Alcántara (ralcantara) Date: 2011-06-30 10:29 Message: Hi, Egon. My code is quite similar: IMolecule mol = builder.newInstance(IMolecule.class); mol.addAtom(new Atom("He")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(mol); Assert.assertEquals("He", MolecularFormulaManipulator.getString(formula)); Regards, Rafael ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 17:17 Message: Rafael, does your code look like this: IAtomContainer helium = new Molecule(); helium.addAtom(new Atom("Am")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(helium); Assert.assertNotNull(formula); Assert.assertEquals("Am", MolecularFormulaManipulator.getString(formula)); ? ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 15:04 Message: Did you also write a few unit tests? ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-29 15:00 Message: Attached are patches adding the missing elements. Missing were Helium and the tail-end of the periodic table, starting from Americium/Am. I added all elements up to and including Copernicium (#112) Whitespace was a mess (format changing _within_ lines), so I cleaned that up first in a separate patch. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&group_id=20024 |
From: SourceForge.net <no...@so...> - 2011-08-10 09:41:00
|
Patches item #3340660, was opened at 2011-06-28 15:02 Message generated for change (Comment added) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&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: Fixed Priority: 9 Private: No Submitted By: Rafael Alcántara (ralcantara) Assigned to: Nobody/Anonymous (nobody) Summary: No formula generated when certain elements present Initial Comment: In some cases, when there is a Helium atom in a molecule (ex. http://www.ebi.ac.uk/chebi/searchId.do?chebiId=33685) the formula is not computed. This is due to that element being missing in org.openscience.cdk.tools.manipulator.MolecularFormulaManipulator.generateOrderEle* methods. The problem applies also to other elements: He, Rf, Db, Sg, Bh, Hs, Mt, Ds, Rg, Am, Cm, Bk, Cf, Es, Fm, Md, No, Lr, Uub ---------------------------------------------------------------------- >Comment By: Egon Willighagen (egonw) Date: 2011-08-10 11:41 Message: Jules, I did not get those whitespace patches to apply cleanly. I've skipped those, and applied 0005 and 0006. The latter with some reluctance, and the JavaDoc is nearly OK. It should in principle have periods after the first sentence, so this will trigger OpenJavaDocCheck changes, but combining this is not overly user-visible (it's test classes), and an improvement of what there was, I'll apply anyway. But, feel most invited to add those missing periods :) ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-07-04 11:06 Message: The testclass-patches (0004-0006) are based on a clean 1.4.0; 0007 is an update to be applied after 0002, if we care about tabs-not-being-spaces ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-04 10:54 Message: Are those updated patches, or based on CDK 1.4.0 ? ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-07-04 10:33 Message: re-opening to fully see it through. - updated regressed unit test (0005) - fixed whitespace for testclass first (0004) - removed meaningless test-class javadoc, wrote new oneliners where useful (0006) - changed tabs to spaces for MolecularFormulaManipulator (0007) ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-03 09:11 Message: BTW, the fix caused a regression: https://sourceforge.net/tracker/?func=detail&aid=3349469&group_id=20024&atid=120024 that unit test got outdated, and need updating. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-02 20:38 Message: Thanx for the reviewing. Applied and pushed. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-02 19:13 Message: Spaces. ---------------------------------------------------------------------- Comment By: gilleain maclean torrance (gilleain) Date: 2011-07-02 18:47 Message: Looks good. I approve of whitespace cleaning (and the alignment of element symbols in the array, to improve readability). Also good that this is a separate patch. One thing, though, does the CDK use tabs or spaces? I prefer spaces... ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-30 11:35 Message: Egon, I couldn't figure out how to unit-test a list of strings, essentially a constant. If you have suggestions on how to do this, I'll gladly write some tests. Added bonus: if new elements get accepted, we'll need to update it again. I chose to leave out the Uup, Uuh, etc. elements since you won't encounter those in 'normal' chemistry. (The same may be said for most of row 7, but ah well) ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:40 Message: Should be part of 1.4.0. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:38 Message: Cool! That was actually the second unit test in my patch :) ---------------------------------------------------------------------- Comment By: Rafael Alcántara (ralcantara) Date: 2011-06-30 10:29 Message: Hi, Egon. My code is quite similar: IMolecule mol = builder.newInstance(IMolecule.class); mol.addAtom(new Atom("He")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(mol); Assert.assertEquals("He", MolecularFormulaManipulator.getString(formula)); Regards, Rafael ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 17:17 Message: Rafael, does your code look like this: IAtomContainer helium = new Molecule(); helium.addAtom(new Atom("Am")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(helium); Assert.assertNotNull(formula); Assert.assertEquals("Am", MolecularFormulaManipulator.getString(formula)); ? ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 15:04 Message: Did you also write a few unit tests? ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-29 15:00 Message: Attached are patches adding the missing elements. Missing were Helium and the tail-end of the periodic table, starting from Americium/Am. I added all elements up to and including Copernicium (#112) Whitespace was a mess (format changing _within_ lines), so I cleaned that up first in a separate patch. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&group_id=20024 |
From: SourceForge.net <no...@so...> - 2011-08-10 09:47:00
|
Patches item #3340660, was opened at 2011-06-28 15:02 Message generated for change (Settings changed) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&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: Accepted >Status: Closed Resolution: Fixed Priority: 9 Private: No Submitted By: Rafael Alcántara (ralcantara) Assigned to: Nobody/Anonymous (nobody) Summary: No formula generated when certain elements present Initial Comment: In some cases, when there is a Helium atom in a molecule (ex. http://www.ebi.ac.uk/chebi/searchId.do?chebiId=33685) the formula is not computed. This is due to that element being missing in org.openscience.cdk.tools.manipulator.MolecularFormulaManipulator.generateOrderEle* methods. The problem applies also to other elements: He, Rf, Db, Sg, Bh, Hs, Mt, Ds, Rg, Am, Cm, Bk, Cf, Es, Fm, Md, No, Lr, Uub ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-08-10 11:41 Message: Jules, I did not get those whitespace patches to apply cleanly. I've skipped those, and applied 0005 and 0006. The latter with some reluctance, and the JavaDoc is nearly OK. It should in principle have periods after the first sentence, so this will trigger OpenJavaDocCheck changes, but combining this is not overly user-visible (it's test classes), and an improvement of what there was, I'll apply anyway. But, feel most invited to add those missing periods :) ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-07-04 11:06 Message: The testclass-patches (0004-0006) are based on a clean 1.4.0; 0007 is an update to be applied after 0002, if we care about tabs-not-being-spaces ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-04 10:54 Message: Are those updated patches, or based on CDK 1.4.0 ? ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-07-04 10:33 Message: re-opening to fully see it through. - updated regressed unit test (0005) - fixed whitespace for testclass first (0004) - removed meaningless test-class javadoc, wrote new oneliners where useful (0006) - changed tabs to spaces for MolecularFormulaManipulator (0007) ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-03 09:11 Message: BTW, the fix caused a regression: https://sourceforge.net/tracker/?func=detail&aid=3349469&group_id=20024&atid=120024 that unit test got outdated, and need updating. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-02 20:38 Message: Thanx for the reviewing. Applied and pushed. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-07-02 19:13 Message: Spaces. ---------------------------------------------------------------------- Comment By: gilleain maclean torrance (gilleain) Date: 2011-07-02 18:47 Message: Looks good. I approve of whitespace cleaning (and the alignment of element symbols in the array, to improve readability). Also good that this is a separate patch. One thing, though, does the CDK use tabs or spaces? I prefer spaces... ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-30 11:35 Message: Egon, I couldn't figure out how to unit-test a list of strings, essentially a constant. If you have suggestions on how to do this, I'll gladly write some tests. Added bonus: if new elements get accepted, we'll need to update it again. I chose to leave out the Uup, Uuh, etc. elements since you won't encounter those in 'normal' chemistry. (The same may be said for most of row 7, but ah well) ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:40 Message: Should be part of 1.4.0. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-30 10:38 Message: Cool! That was actually the second unit test in my patch :) ---------------------------------------------------------------------- Comment By: Rafael Alcántara (ralcantara) Date: 2011-06-30 10:29 Message: Hi, Egon. My code is quite similar: IMolecule mol = builder.newInstance(IMolecule.class); mol.addAtom(new Atom("He")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(mol); Assert.assertEquals("He", MolecularFormulaManipulator.getString(formula)); Regards, Rafael ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 17:17 Message: Rafael, does your code look like this: IAtomContainer helium = new Molecule(); helium.addAtom(new Atom("Am")); IMolecularFormula formula = MolecularFormulaManipulator.getMolecularFormula(helium); Assert.assertNotNull(formula); Assert.assertEquals("Am", MolecularFormulaManipulator.getString(formula)); ? ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2011-06-29 15:04 Message: Did you also write a few unit tests? ---------------------------------------------------------------------- Comment By: Jules Kerssemakers (jkerssem) Date: 2011-06-29 15:00 Message: Attached are patches adding the missing elements. Missing were Helium and the tail-end of the periodic table, starting from Americium/Am. I added all elements up to and including Copernicium (#112) Whitespace was a mess (format changing _within_ lines), so I cleaned that up first in a separate patch. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3340660&group_id=20024 |