From: SourceForge.net <no...@so...> - 2009-11-15 21:51:18
|
Patches item #2688082, was opened at 2009-03-15 20:00 Message generated for change (Comment added) made by rajarshi You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2688082&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: Deleted >Resolution: Out of Date Priority: 5 Private: No Submitted By: Rajarshi Guha (rajarshi) Assigned to: Rajarshi Guha (rajarshi) Summary: Consolidate and cleanup the periodic table code Initial Comment: 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-15 16:51 Message: Closing this out, since it is superceded by a recent patch set ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-09-20 14:24 Message: It's more that I want to make sure that no other changes have been made to the modified code in between... and I am not sure if the merge is not overwriting any other fixes to the code... I'll check the logs this week, and if I don't find anything, you can go ahead. ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2009-09-20 13:30 Message: Given that it's been reviewed, can't I just commit from my side - yes, it goes against the policies, but given the hassle of fixing these conflicts, it might be easier to do it this once (and in the future try and keep up with the patches, so they don't fall too far behind) ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-09-20 04:39 Message: No, it just doesn't work... the problem is the moving of files... that's what makes it complex. Have you tried rebasing the patches on top of the new master? ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-09-20 02:10 Message: I fixed bits, but the copying of files went wrong, because I redid that. I can try again and send you the 'new' patches... ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2009-09-19 18:44 Message: Was this all fixed? Or are problems remaining? ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-09-19 10:43 Message: Yeah, I tried to manually fix many of them... that works at least well for cdk.svnrev to cdk.githash... But I think it went wrong with some of the copied files, which I thought were mere copying, but seems to have been changes as well... ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2009-09-19 10:21 Message: Hmm, that's the problem. In my branch (cleanpt) I do git format-patch master..cleanpt which gives me all 14 patches. But sicne they were put in a long time ago, when master had @cdk.svnrev tags, they show up in the patches. I can't see a way to make a single monlithic patch bettween the current master HEAD and my cleanpt branch head ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-09-19 10:08 Message: Mmm... did you tar up the right patches? They still do not apply for me... one of the problems is that the patches have @cdk.svnrev, which make them not apply in my master... ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2009-09-19 09:28 Message: I worked out how to generate the patch, so a tarball with 14 patches has been uploaded ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2009-09-19 09:16 Message: I have updated the code to work with latest master. However, as it was a merge, I can't seem to make a patch file (git format-patch XXXX). Do you know how to make one for this case? or esle, I could just go ahead and commit this to master ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-09-18 19:12 Message: Rajarshi, please update this patch against the current master. Too many conflicts I cannot solve myself. Otherwise, patch looks fine, though while at it... please use non-wild-care imports. ---------------------------------------------------------------------- Comment By: Stefan Kuhn (shk3) Date: 2009-06-09 11:29 Message: Good cleanup, no problems with that. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2009-04-12 18:15 Message: Rajarshi asked me to put up the branch on a machine here in Uppsala, as his ath.cx machine is going down, so please check the patch from: git pull http://pele.farmbio.uu.se/git/rajarshi.git/ cleanpt ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2688082&group_id=20024 |