From: SourceForge.net <no...@so...> - 2009-11-15 21:49:43
|
Patches item #2898194, was opened at 2009-11-15 16:49 Message generated for change (Tracker Item Submitted) made by rajarshi You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2898194&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: Needs Review Status: Open Resolution: None Priority: 6 Private: No Submitted By: Rajarshi Guha (rajarshi) Assigned to: Egon Willighagen (egonw) Summary: periodic table refactoring Initial Comment: This is a fresh redo of the periodic table refctoring - move all PT classes into their own class, make a single class publicly acessible and remove the Symbols class and all its usages. This is based on the latest master. I've closed out the original patch for this issue, and copied the summary from there. Goal ---- Consolidate and cleanup the periodic table (PT) handling code and prepare for inclusion in the standard module. Availability --------- This update should be considered for CDK master (and not 1.2.x since it has API changes) You can pull the update by git pull git://rguha.ath.cx/cdk cleanpt Background ---------- The goal of the PT classes is to provide a static class to provide 'constant' element information. In this context constant information refers to things such as symbols, atomic numbers, VdW radii and so on. These data are taken from the BODR repository and are characteristic of a given element. This suggests that it be available from a singleton type class. Current Problems -------------- 1. Multiple overlapping classes - the Symbols class allows symbol lookup via atomic number and vice versa. The PeriodicTable class also allows these two operations, along with methods to get VdW radii, electronegativity etc. Also, one can get a PeriodicTableElement and retrieve these items as well. Thus we have 3 classes with overlapping functionalty. 2. Classes used purely internally are public and therefore pollute the public API. Specifically, the classes to load the PT data from the XML files are public, but are *only* used internally. I don't see a need for them to be used externally. This applies to classes such as ElementPTFactory and its dependent classes. Also, PeriodicTableElement is not used anywhere in the CDK. This suggests that it be converted to a private class. 3. Presence of PeriodicTableElement - given that common usage of PT information is in terms of specific values (i.e., get the symbol or VdW radii or atomic number etc), there doesn't seem to be a specific need for a element of the PT. 4. Since the different classes making up the PT infrastructure are dsitributed amongst multiple packages, they are all *forced* to be public, polluting the public API. 5. (Minor) The PT related classes should be part of standard Solution ------- Since PeriodicTable is already present and implements a lazy initialization scheme, this class aims to be the place to get PT related information. The current update simply relocates some classes and updates code that used the current PeriodicTable or Symbols classes. 1. Refactored code so that all PT related classes are now under org.openscience.cdk.tools.periodictable 2. Make PeriodicTable public and all the rest are package private (i.e., no explicit class modifier). As a result, a user of the library only sees PeriodicTable and none of the internal helper classes 3. Similarly refactor the tests into the appropriate package 4. Update all code to use the new PeriodicTable. For code using Symbols, update them to use PeriodicTable rather than Symbols 5. Updated module membership of PT classes to standard - this is primarily to let other modules compile, though the long term goal is to make the PT classes part of standard. As a result, tests are also part of standard Results ------ 1. Cleanly compiles 2. No new unit test fails Objections --------- Possible objections include: 1. ElementPTFactory is a config related class so should be in package config I don't think this is a big deal - by keeping the PT related classes together the code base is cleaner. Also, the current solution allows us to hide this internal class from users of the library 2. The Symbols class used array indexing to retrieve a symbol based on atomic number. PeriodicTable uses a hash table lookup. While there is some loss of performance I don't think it is much (but this is not a rigorous answer). In the worst case, the PeriodicTable class can be refactored to employ array lookups rather than hash lookups for certain operations (i.e. those indexed by atomic number) 3. Somebody will want to use PeriodicTableElement I can't see a valid use case for such a class. It seems that any code that needs to use such a class can be easily refactored to make calls to the PeriodicTable class. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2898194&group_id=20024 |
From: SourceForge.net <no...@so...> - 2009-11-23 08:25:58
|
Patches item #2898194, was opened at 2009-11-15 22:49 Message generated for change (Comment added) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2898194&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: Needs Review Status: Open Resolution: None Priority: 6 Private: No Submitted By: Rajarshi Guha (rajarshi) Assigned to: Egon Willighagen (egonw) Summary: periodic table refactoring Initial Comment: This is a fresh redo of the periodic table refctoring - move all PT classes into their own class, make a single class publicly acessible and remove the Symbols class and all its usages. This is based on the latest master. I've closed out the original patch for this issue, and copied the summary from there. Goal ---- Consolidate and cleanup the periodic table (PT) handling code and prepare for inclusion in the standard module. Availability --------- This update should be considered for CDK master (and not 1.2.x since it has API changes) You can pull the update by git pull git://rguha.ath.cx/cdk cleanpt Background ---------- The goal of the PT classes is to provide a static class to provide 'constant' element information. In this context constant information refers to things such as symbols, atomic numbers, VdW radii and so on. These data are taken from the BODR repository and are characteristic of a given element. This suggests that it be available from a singleton type class. Current Problems -------------- 1. Multiple overlapping classes - the Symbols class allows symbol lookup via atomic number and vice versa. The PeriodicTable class also allows these two operations, along with methods to get VdW radii, electronegativity etc. Also, one can get a PeriodicTableElement and retrieve these items as well. Thus we have 3 classes with overlapping functionalty. 2. Classes used purely internally are public and therefore pollute the public API. Specifically, the classes to load the PT data from the XML files are public, but are *only* used internally. I don't see a need for them to be used externally. This applies to classes such as ElementPTFactory and its dependent classes. Also, PeriodicTableElement is not used anywhere in the CDK. This suggests that it be converted to a private class. 3. Presence of PeriodicTableElement - given that common usage of PT information is in terms of specific values (i.e., get the symbol or VdW radii or atomic number etc), there doesn't seem to be a specific need for a element of the PT. 4. Since the different classes making up the PT infrastructure are dsitributed amongst multiple packages, they are all *forced* to be public, polluting the public API. 5. (Minor) The PT related classes should be part of standard Solution ------- Since PeriodicTable is already present and implements a lazy initialization scheme, this class aims to be the place to get PT related information. The current update simply relocates some classes and updates code that used the current PeriodicTable or Symbols classes. 1. Refactored code so that all PT related classes are now under org.openscience.cdk.tools.periodictable 2. Make PeriodicTable public and all the rest are package private (i.e., no explicit class modifier). As a result, a user of the library only sees PeriodicTable and none of the internal helper classes 3. Similarly refactor the tests into the appropriate package 4. Update all code to use the new PeriodicTable. For code using Symbols, update them to use PeriodicTable rather than Symbols 5. Updated module membership of PT classes to standard - this is primarily to let other modules compile, though the long term goal is to make the PT classes part of standard. As a result, tests are also part of standard Results ------ 1. Cleanly compiles 2. No new unit test fails Objections --------- Possible objections include: 1. ElementPTFactory is a config related class so should be in package config I don't think this is a big deal - by keeping the PT related classes together the code base is cleaner. Also, the current solution allows us to hide this internal class from users of the library 2. The Symbols class used array indexing to retrieve a symbol based on atomic number. PeriodicTable uses a hash table lookup. While there is some loss of performance I don't think it is much (but this is not a rigorous answer). In the worst case, the PeriodicTable class can be refactored to employ array lookups rather than hash lookups for certain operations (i.e. those indexed by atomic number) 3. Somebody will want to use PeriodicTableElement I can't see a valid use case for such a class. It seems that any code that needs to use such a class can be easily refactored to make calls to the PeriodicTable class. ---------------------------------------------------------------------- >Comment By: Egon Willighagen (egonw) Date: 2009-11-23 09:25 Message: Rajarshi, code looks fairly good, though have not run all quality assurance... However, there is one big blocker for the current patch: [javac] /home/egonw/var/Projects/SourceForge/git/cdk/build/src/standard/org/openscience/cdk/tools/periodictable/PeriodicTableElement.java:28: cannot find symbol [javac] symbol : class Element [javac] location: package org.openscience.cdk [javac] import org.openscience.cdk.Element; [javac] ^ The 'standard' module cannot depend on the 'data' module. There is no easy trick here... I am working on an improved IChemObjectBuilder system, but you will have to implement a custom IElement implementation (feel to just copy the current Element code, and rip out the notification stuff)... since this class is private, I suggest to return null for .getBuilder() ? ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2898194&group_id=20024 |
From: SourceForge.net <no...@so...> - 2009-11-23 09:44:54
|
Patches item #2898194, was opened at 2009-11-15 22:49 Message generated for change (Comment added) made by shk3 You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2898194&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: Needs Review Status: Open Resolution: None Priority: 6 Private: No Submitted By: Rajarshi Guha (rajarshi) Assigned to: Egon Willighagen (egonw) Summary: periodic table refactoring Initial Comment: This is a fresh redo of the periodic table refctoring - move all PT classes into their own class, make a single class publicly acessible and remove the Symbols class and all its usages. This is based on the latest master. I've closed out the original patch for this issue, and copied the summary from there. Goal ---- Consolidate and cleanup the periodic table (PT) handling code and prepare for inclusion in the standard module. Availability --------- This update should be considered for CDK master (and not 1.2.x since it has API changes) You can pull the update by git pull git://rguha.ath.cx/cdk cleanpt Background ---------- The goal of the PT classes is to provide a static class to provide 'constant' element information. In this context constant information refers to things such as symbols, atomic numbers, VdW radii and so on. These data are taken from the BODR repository and are characteristic of a given element. This suggests that it be available from a singleton type class. Current Problems -------------- 1. Multiple overlapping classes - the Symbols class allows symbol lookup via atomic number and vice versa. The PeriodicTable class also allows these two operations, along with methods to get VdW radii, electronegativity etc. Also, one can get a PeriodicTableElement and retrieve these items as well. Thus we have 3 classes with overlapping functionalty. 2. Classes used purely internally are public and therefore pollute the public API. Specifically, the classes to load the PT data from the XML files are public, but are *only* used internally. I don't see a need for them to be used externally. This applies to classes such as ElementPTFactory and its dependent classes. Also, PeriodicTableElement is not used anywhere in the CDK. This suggests that it be converted to a private class. 3. Presence of PeriodicTableElement - given that common usage of PT information is in terms of specific values (i.e., get the symbol or VdW radii or atomic number etc), there doesn't seem to be a specific need for a element of the PT. 4. Since the different classes making up the PT infrastructure are dsitributed amongst multiple packages, they are all *forced* to be public, polluting the public API. 5. (Minor) The PT related classes should be part of standard Solution ------- Since PeriodicTable is already present and implements a lazy initialization scheme, this class aims to be the place to get PT related information. The current update simply relocates some classes and updates code that used the current PeriodicTable or Symbols classes. 1. Refactored code so that all PT related classes are now under org.openscience.cdk.tools.periodictable 2. Make PeriodicTable public and all the rest are package private (i.e., no explicit class modifier). As a result, a user of the library only sees PeriodicTable and none of the internal helper classes 3. Similarly refactor the tests into the appropriate package 4. Update all code to use the new PeriodicTable. For code using Symbols, update them to use PeriodicTable rather than Symbols 5. Updated module membership of PT classes to standard - this is primarily to let other modules compile, though the long term goal is to make the PT classes part of standard. As a result, tests are also part of standard Results ------ 1. Cleanly compiles 2. No new unit test fails Objections --------- Possible objections include: 1. ElementPTFactory is a config related class so should be in package config I don't think this is a big deal - by keeping the PT related classes together the code base is cleaner. Also, the current solution allows us to hide this internal class from users of the library 2. The Symbols class used array indexing to retrieve a symbol based on atomic number. PeriodicTable uses a hash table lookup. While there is some loss of performance I don't think it is much (but this is not a rigorous answer). In the worst case, the PeriodicTable class can be refactored to employ array lookups rather than hash lookups for certain operations (i.e. those indexed by atomic number) 3. Somebody will want to use PeriodicTableElement I can't see a valid use case for such a class. It seems that any code that needs to use such a class can be easily refactored to make calls to the PeriodicTable class. ---------------------------------------------------------------------- >Comment By: Stefan Kuhn (shk3) Date: 2009-11-23 10:44 Message: I think a refactoring here should also address the i18l issue. Right now, getName of PTElement gives an English name and the chemicalElements.xml contains cdk:name="", which is actually an English name. Btw, there is also an elements.owl file, which seems to be prepared to take different languages. I think the chemicalElements.xml should be similar in that respect and there should be a getName(String language) method (getName() should probably default to English). This is needed to actually localize the periodic table in jcp, but might be relevant in other cases as well. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-11-23 09:25 Message: Rajarshi, code looks fairly good, though have not run all quality assurance... However, there is one big blocker for the current patch: [javac] /home/egonw/var/Projects/SourceForge/git/cdk/build/src/standard/org/openscience/cdk/tools/periodictable/PeriodicTableElement.java:28: cannot find symbol [javac] symbol : class Element [javac] location: package org.openscience.cdk [javac] import org.openscience.cdk.Element; [javac] ^ The 'standard' module cannot depend on the 'data' module. There is no easy trick here... I am working on an improved IChemObjectBuilder system, but you will have to implement a custom IElement implementation (feel to just copy the current Element code, and rip out the notification stuff)... since this class is private, I suggest to return null for .getBuilder() ? ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2898194&group_id=20024 |
From: SourceForge.net <no...@so...> - 2009-11-23 12:53:17
|
Patches item #2898194, was opened at 2009-11-15 22:49 Message generated for change (Comment added) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2898194&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: Needs Review Status: Open Resolution: None Priority: 6 Private: No Submitted By: Rajarshi Guha (rajarshi) Assigned to: Egon Willighagen (egonw) Summary: periodic table refactoring Initial Comment: This is a fresh redo of the periodic table refctoring - move all PT classes into their own class, make a single class publicly acessible and remove the Symbols class and all its usages. This is based on the latest master. I've closed out the original patch for this issue, and copied the summary from there. Goal ---- Consolidate and cleanup the periodic table (PT) handling code and prepare for inclusion in the standard module. Availability --------- This update should be considered for CDK master (and not 1.2.x since it has API changes) You can pull the update by git pull git://rguha.ath.cx/cdk cleanpt Background ---------- The goal of the PT classes is to provide a static class to provide 'constant' element information. In this context constant information refers to things such as symbols, atomic numbers, VdW radii and so on. These data are taken from the BODR repository and are characteristic of a given element. This suggests that it be available from a singleton type class. Current Problems -------------- 1. Multiple overlapping classes - the Symbols class allows symbol lookup via atomic number and vice versa. The PeriodicTable class also allows these two operations, along with methods to get VdW radii, electronegativity etc. Also, one can get a PeriodicTableElement and retrieve these items as well. Thus we have 3 classes with overlapping functionalty. 2. Classes used purely internally are public and therefore pollute the public API. Specifically, the classes to load the PT data from the XML files are public, but are *only* used internally. I don't see a need for them to be used externally. This applies to classes such as ElementPTFactory and its dependent classes. Also, PeriodicTableElement is not used anywhere in the CDK. This suggests that it be converted to a private class. 3. Presence of PeriodicTableElement - given that common usage of PT information is in terms of specific values (i.e., get the symbol or VdW radii or atomic number etc), there doesn't seem to be a specific need for a element of the PT. 4. Since the different classes making up the PT infrastructure are dsitributed amongst multiple packages, they are all *forced* to be public, polluting the public API. 5. (Minor) The PT related classes should be part of standard Solution ------- Since PeriodicTable is already present and implements a lazy initialization scheme, this class aims to be the place to get PT related information. The current update simply relocates some classes and updates code that used the current PeriodicTable or Symbols classes. 1. Refactored code so that all PT related classes are now under org.openscience.cdk.tools.periodictable 2. Make PeriodicTable public and all the rest are package private (i.e., no explicit class modifier). As a result, a user of the library only sees PeriodicTable and none of the internal helper classes 3. Similarly refactor the tests into the appropriate package 4. Update all code to use the new PeriodicTable. For code using Symbols, update them to use PeriodicTable rather than Symbols 5. Updated module membership of PT classes to standard - this is primarily to let other modules compile, though the long term goal is to make the PT classes part of standard. As a result, tests are also part of standard Results ------ 1. Cleanly compiles 2. No new unit test fails Objections --------- Possible objections include: 1. ElementPTFactory is a config related class so should be in package config I don't think this is a big deal - by keeping the PT related classes together the code base is cleaner. Also, the current solution allows us to hide this internal class from users of the library 2. The Symbols class used array indexing to retrieve a symbol based on atomic number. PeriodicTable uses a hash table lookup. While there is some loss of performance I don't think it is much (but this is not a rigorous answer). In the worst case, the PeriodicTable class can be refactored to employ array lookups rather than hash lookups for certain operations (i.e. those indexed by atomic number) 3. Somebody will want to use PeriodicTableElement I can't see a valid use case for such a class. It seems that any code that needs to use such a class can be easily refactored to make calls to the PeriodicTable class. ---------------------------------------------------------------------- >Comment By: Egon Willighagen (egonw) Date: 2009-11-23 13:53 Message: Stefan, I agree that i18n should indeed be addressed at a CDK level. Because doing that actually involves some careful thinking, I think this patch is not the place to do that. To implement i18n we need to first work around the current build system, and have translations at a module level. A very long time ago I briefly pondered with that idea, but JChemPaint is now a clear example this is really needed (and I 2.6 should also bring i18n to Bioclipse)... Can you please take this to the cdk-devel mailing list, and start the discussion? Can you add your thoughts on using the GnuText approach, or the native Java approach? The former seems to work well for the JChemPaint applet, or ... ? ---------------------------------------------------------------------- Comment By: Stefan Kuhn (shk3) Date: 2009-11-23 10:44 Message: I think a refactoring here should also address the i18l issue. Right now, getName of PTElement gives an English name and the chemicalElements.xml contains cdk:name="", which is actually an English name. Btw, there is also an elements.owl file, which seems to be prepared to take different languages. I think the chemicalElements.xml should be similar in that respect and there should be a getName(String language) method (getName() should probably default to English). This is needed to actually localize the periodic table in jcp, but might be relevant in other cases as well. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-11-23 09:25 Message: Rajarshi, code looks fairly good, though have not run all quality assurance... However, there is one big blocker for the current patch: [javac] /home/egonw/var/Projects/SourceForge/git/cdk/build/src/standard/org/openscience/cdk/tools/periodictable/PeriodicTableElement.java:28: cannot find symbol [javac] symbol : class Element [javac] location: package org.openscience.cdk [javac] import org.openscience.cdk.Element; [javac] ^ The 'standard' module cannot depend on the 'data' module. There is no easy trick here... I am working on an improved IChemObjectBuilder system, but you will have to implement a custom IElement implementation (feel to just copy the current Element code, and rip out the notification stuff)... since this class is private, I suggest to return null for .getBuilder() ? ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2898194&group_id=20024 |
From: SourceForge.net <no...@so...> - 2009-11-26 21:48:06
|
Patches item #2898194, was opened at 2009-11-15 16:49 Message generated for change (Comment added) made by rajarshi You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2898194&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: Needs Review Status: Open Resolution: None Priority: 6 Private: No Submitted By: Rajarshi Guha (rajarshi) Assigned to: Egon Willighagen (egonw) Summary: periodic table refactoring Initial Comment: This is a fresh redo of the periodic table refctoring - move all PT classes into their own class, make a single class publicly acessible and remove the Symbols class and all its usages. This is based on the latest master. I've closed out the original patch for this issue, and copied the summary from there. Goal ---- Consolidate and cleanup the periodic table (PT) handling code and prepare for inclusion in the standard module. Availability --------- This update should be considered for CDK master (and not 1.2.x since it has API changes) You can pull the update by git pull git://rguha.ath.cx/cdk cleanpt Background ---------- The goal of the PT classes is to provide a static class to provide 'constant' element information. In this context constant information refers to things such as symbols, atomic numbers, VdW radii and so on. These data are taken from the BODR repository and are characteristic of a given element. This suggests that it be available from a singleton type class. Current Problems -------------- 1. Multiple overlapping classes - the Symbols class allows symbol lookup via atomic number and vice versa. The PeriodicTable class also allows these two operations, along with methods to get VdW radii, electronegativity etc. Also, one can get a PeriodicTableElement and retrieve these items as well. Thus we have 3 classes with overlapping functionalty. 2. Classes used purely internally are public and therefore pollute the public API. Specifically, the classes to load the PT data from the XML files are public, but are *only* used internally. I don't see a need for them to be used externally. This applies to classes such as ElementPTFactory and its dependent classes. Also, PeriodicTableElement is not used anywhere in the CDK. This suggests that it be converted to a private class. 3. Presence of PeriodicTableElement - given that common usage of PT information is in terms of specific values (i.e., get the symbol or VdW radii or atomic number etc), there doesn't seem to be a specific need for a element of the PT. 4. Since the different classes making up the PT infrastructure are dsitributed amongst multiple packages, they are all *forced* to be public, polluting the public API. 5. (Minor) The PT related classes should be part of standard Solution ------- Since PeriodicTable is already present and implements a lazy initialization scheme, this class aims to be the place to get PT related information. The current update simply relocates some classes and updates code that used the current PeriodicTable or Symbols classes. 1. Refactored code so that all PT related classes are now under org.openscience.cdk.tools.periodictable 2. Make PeriodicTable public and all the rest are package private (i.e., no explicit class modifier). As a result, a user of the library only sees PeriodicTable and none of the internal helper classes 3. Similarly refactor the tests into the appropriate package 4. Update all code to use the new PeriodicTable. For code using Symbols, update them to use PeriodicTable rather than Symbols 5. Updated module membership of PT classes to standard - this is primarily to let other modules compile, though the long term goal is to make the PT classes part of standard. As a result, tests are also part of standard Results ------ 1. Cleanly compiles 2. No new unit test fails Objections --------- Possible objections include: 1. ElementPTFactory is a config related class so should be in package config I don't think this is a big deal - by keeping the PT related classes together the code base is cleaner. Also, the current solution allows us to hide this internal class from users of the library 2. The Symbols class used array indexing to retrieve a symbol based on atomic number. PeriodicTable uses a hash table lookup. While there is some loss of performance I don't think it is much (but this is not a rigorous answer). In the worst case, the PeriodicTable class can be refactored to employ array lookups rather than hash lookups for certain operations (i.e. those indexed by atomic number) 3. Somebody will want to use PeriodicTableElement I can't see a valid use case for such a class. It seems that any code that needs to use such a class can be easily refactored to make calls to the PeriodicTable class. ---------------------------------------------------------------------- >Comment By: Rajarshi Guha (rajarshi) Date: 2009-11-26 16:48 Message: Regarding the issue of the periodic table classes being independent of the data module, the most recent tgz (cleanpt-patches-2.tgz) contains the patches to fix this. Basically PeriodicTableElement is now standalone class - doesn't subclass anything. This is OK since 1) it is private to the package and 2) it's only purpose is to store the various peices of information regarding periodic table information for an eleemnt. So at this point, the refactoring should be independent of the data module ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-11-23 07:53 Message: Stefan, I agree that i18n should indeed be addressed at a CDK level. Because doing that actually involves some careful thinking, I think this patch is not the place to do that. To implement i18n we need to first work around the current build system, and have translations at a module level. A very long time ago I briefly pondered with that idea, but JChemPaint is now a clear example this is really needed (and I 2.6 should also bring i18n to Bioclipse)... Can you please take this to the cdk-devel mailing list, and start the discussion? Can you add your thoughts on using the GnuText approach, or the native Java approach? The former seems to work well for the JChemPaint applet, or ... ? ---------------------------------------------------------------------- Comment By: Stefan Kuhn (shk3) Date: 2009-11-23 04:44 Message: I think a refactoring here should also address the i18l issue. Right now, getName of PTElement gives an English name and the chemicalElements.xml contains cdk:name="", which is actually an English name. Btw, there is also an elements.owl file, which seems to be prepared to take different languages. I think the chemicalElements.xml should be similar in that respect and there should be a getName(String language) method (getName() should probably default to English). This is needed to actually localize the periodic table in jcp, but might be relevant in other cases as well. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-11-23 03:25 Message: Rajarshi, code looks fairly good, though have not run all quality assurance... However, there is one big blocker for the current patch: [javac] /home/egonw/var/Projects/SourceForge/git/cdk/build/src/standard/org/openscience/cdk/tools/periodictable/PeriodicTableElement.java:28: cannot find symbol [javac] symbol : class Element [javac] location: package org.openscience.cdk [javac] import org.openscience.cdk.Element; [javac] ^ The 'standard' module cannot depend on the 'data' module. There is no easy trick here... I am working on an improved IChemObjectBuilder system, but you will have to implement a custom IElement implementation (feel to just copy the current Element code, and rip out the notification stuff)... since this class is private, I suggest to return null for .getBuilder() ? ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2898194&group_id=20024 |
From: SourceForge.net <no...@so...> - 2009-11-29 10:48:50
|
Patches item #2898194, was opened at 2009-11-15 22:49 Message generated for change (Comment added) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2898194&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: Accepted >Status: Closed >Resolution: Fixed Priority: 6 Private: No Submitted By: Rajarshi Guha (rajarshi) Assigned to: Egon Willighagen (egonw) Summary: periodic table refactoring Initial Comment: This is a fresh redo of the periodic table refctoring - move all PT classes into their own class, make a single class publicly acessible and remove the Symbols class and all its usages. This is based on the latest master. I've closed out the original patch for this issue, and copied the summary from there. Goal ---- Consolidate and cleanup the periodic table (PT) handling code and prepare for inclusion in the standard module. Availability --------- This update should be considered for CDK master (and not 1.2.x since it has API changes) You can pull the update by git pull git://rguha.ath.cx/cdk cleanpt Background ---------- The goal of the PT classes is to provide a static class to provide 'constant' element information. In this context constant information refers to things such as symbols, atomic numbers, VdW radii and so on. These data are taken from the BODR repository and are characteristic of a given element. This suggests that it be available from a singleton type class. Current Problems -------------- 1. Multiple overlapping classes - the Symbols class allows symbol lookup via atomic number and vice versa. The PeriodicTable class also allows these two operations, along with methods to get VdW radii, electronegativity etc. Also, one can get a PeriodicTableElement and retrieve these items as well. Thus we have 3 classes with overlapping functionalty. 2. Classes used purely internally are public and therefore pollute the public API. Specifically, the classes to load the PT data from the XML files are public, but are *only* used internally. I don't see a need for them to be used externally. This applies to classes such as ElementPTFactory and its dependent classes. Also, PeriodicTableElement is not used anywhere in the CDK. This suggests that it be converted to a private class. 3. Presence of PeriodicTableElement - given that common usage of PT information is in terms of specific values (i.e., get the symbol or VdW radii or atomic number etc), there doesn't seem to be a specific need for a element of the PT. 4. Since the different classes making up the PT infrastructure are dsitributed amongst multiple packages, they are all *forced* to be public, polluting the public API. 5. (Minor) The PT related classes should be part of standard Solution ------- Since PeriodicTable is already present and implements a lazy initialization scheme, this class aims to be the place to get PT related information. The current update simply relocates some classes and updates code that used the current PeriodicTable or Symbols classes. 1. Refactored code so that all PT related classes are now under org.openscience.cdk.tools.periodictable 2. Make PeriodicTable public and all the rest are package private (i.e., no explicit class modifier). As a result, a user of the library only sees PeriodicTable and none of the internal helper classes 3. Similarly refactor the tests into the appropriate package 4. Update all code to use the new PeriodicTable. For code using Symbols, update them to use PeriodicTable rather than Symbols 5. Updated module membership of PT classes to standard - this is primarily to let other modules compile, though the long term goal is to make the PT classes part of standard. As a result, tests are also part of standard Results ------ 1. Cleanly compiles 2. No new unit test fails Objections --------- Possible objections include: 1. ElementPTFactory is a config related class so should be in package config I don't think this is a big deal - by keeping the PT related classes together the code base is cleaner. Also, the current solution allows us to hide this internal class from users of the library 2. The Symbols class used array indexing to retrieve a symbol based on atomic number. PeriodicTable uses a hash table lookup. While there is some loss of performance I don't think it is much (but this is not a rigorous answer). In the worst case, the PeriodicTable class can be refactored to employ array lookups rather than hash lookups for certain operations (i.e. those indexed by atomic number) 3. Somebody will want to use PeriodicTableElement I can't see a valid use case for such a class. It seems that any code that needs to use such a class can be easily refactored to make calls to the PeriodicTable class. ---------------------------------------------------------------------- >Comment By: Egon Willighagen (egonw) Date: 2009-11-29 11:48 Message: Thanx for the updates. Applied to master. ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2009-11-26 22:48 Message: Regarding the issue of the periodic table classes being independent of the data module, the most recent tgz (cleanpt-patches-2.tgz) contains the patches to fix this. Basically PeriodicTableElement is now standalone class - doesn't subclass anything. This is OK since 1) it is private to the package and 2) it's only purpose is to store the various peices of information regarding periodic table information for an eleemnt. So at this point, the refactoring should be independent of the data module ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-11-23 13:53 Message: Stefan, I agree that i18n should indeed be addressed at a CDK level. Because doing that actually involves some careful thinking, I think this patch is not the place to do that. To implement i18n we need to first work around the current build system, and have translations at a module level. A very long time ago I briefly pondered with that idea, but JChemPaint is now a clear example this is really needed (and I 2.6 should also bring i18n to Bioclipse)... Can you please take this to the cdk-devel mailing list, and start the discussion? Can you add your thoughts on using the GnuText approach, or the native Java approach? The former seems to work well for the JChemPaint applet, or ... ? ---------------------------------------------------------------------- Comment By: Stefan Kuhn (shk3) Date: 2009-11-23 10:44 Message: I think a refactoring here should also address the i18l issue. Right now, getName of PTElement gives an English name and the chemicalElements.xml contains cdk:name="", which is actually an English name. Btw, there is also an elements.owl file, which seems to be prepared to take different languages. I think the chemicalElements.xml should be similar in that respect and there should be a getName(String language) method (getName() should probably default to English). This is needed to actually localize the periodic table in jcp, but might be relevant in other cases as well. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-11-23 09:25 Message: Rajarshi, code looks fairly good, though have not run all quality assurance... However, there is one big blocker for the current patch: [javac] /home/egonw/var/Projects/SourceForge/git/cdk/build/src/standard/org/openscience/cdk/tools/periodictable/PeriodicTableElement.java:28: cannot find symbol [javac] symbol : class Element [javac] location: package org.openscience.cdk [javac] import org.openscience.cdk.Element; [javac] ^ The 'standard' module cannot depend on the 'data' module. There is no easy trick here... I am working on an improved IChemObjectBuilder system, but you will have to implement a custom IElement implementation (feel to just copy the current Element code, and rip out the notification stuff)... since this class is private, I suggest to return null for .getBuilder() ? ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2898194&group_id=20024 |
From: Mark R. <ma...@eb...> - 2009-11-30 14:44:54
|
I am trying to make this patch work with JCP. Indeed (point 3) things need to be refactored. But the JCP code also uses the factory class (now package level so not visible) to configure an element using method public PeriodicTableElement configure(PeriodicTableElement element) How would this have to be done now? PeriodicTable has a private ElementPTFactory but does not expose its methods. So external classes can never get to ElementPTFactory. Mark > Patches item #2898194, was opened at 2009-11-15 22:49 > > 3. Somebody will want to use PeriodicTableElement > > I can't see a valid use case for such a class. It seems that any code > that needs to use such a class can be easily refactored to make calls > to the PeriodicTable class. > |
From: Rajarshi G. <raj...@gm...> - 2009-11-30 14:56:32
|
On Mon, Nov 30, 2009 at 7:32 AM, Mark Rijnbeek <ma...@eb...> wrote: > > I am trying to make this patch work with JCP. Indeed (point 3) things > need to be refactored. But the JCP code also uses the factory class (now > package level so not visible) to configure an element using method > public PeriodicTableElement configure(PeriodicTableElement element) > > How would this have to be done now? PeriodicTable has a private > ElementPTFactory but does not expose its methods. So external classes > can never get to ElementPTFactory. > Correct - the only external class is PeriodicTable. As such, there is no public PeriodicTableElement either. I'm not familiar with the JCP code, but why does JCP use PeriodicTableElement? Can the JCP version of this class populate the fields by calls to PeriodicTable? Does it need PeriodictableElement at all? -- Rajarshi Guha NIH Chemical Genomics Center |