[Cdk-devel] [ cdk-Patches-2688082 ] Consolidate and cleanup the periodic table code

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 454-5900

Patches item #2688082, was opened at 2009-03-16 01:00
Message generated for change (Comment added) made by egonw
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: Needs Revision
Status: Open
Resolution: None
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: Egon Willighagen (egonw)
Date: 2009-09-20 08: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-20 00:44

Message:
Was this all fixed? Or are problems remaining?

----------------------------------------------------------------------

Comment By: Egon Willighagen (egonw)
Date: 2009-09-19 16: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 16: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 16: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 15: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 15: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-19 01: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 17:29

Message:
Good cleanup, no problems with that.

----------------------------------------------------------------------

Comment By: Egon Willighagen (egonw)
Date: 2009-04-13 00: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