phase out using getSymbol() to determine the element of an atom

[egonw]

Recently, the CDK IAtom was updated to combine the atom number and the symbol, making it principally atom number-based. In the past the symbol was the main source of information, and the atom number is not required to be always set. This means that a lot of code can now use the atom number, which is faster.

Therefore, one jobs is to replace the old code with the new code. For example,

atom.getSymbol().equals("H")

can be rewritten as:

atom.getAtomicNumber() == 1

We welcome such patches. A single patch can make sure updates for one or more Java classes. It is appreciated if you check if there is a unit test for the method, ensuring that our test suite can pick up possible regressions (which should not happen in this case, but we never know).

[xperrylinn]

Hi @egonw,

I'd like to make my first contribution. Is this ticket still valid?

[johnmay]

Yep - we have some enum constants for Elements (Elements.Carbon) but I've always thought it would be nice to have the integer constants too.

In other toolkits I use the nice an concise:

if (atom.getElement() == Atom.H) {

}

where H is an integer. In CDK it's a bit more verbose:

if (atom.getAtomicNumber() == Elements.Hydrogen.number()) {

}

I'm not if @egonw has any ideas about an intermediate between these.

[johnmay]

I think adding them to the IAtom interface would be okay:

if (atom.getAtomicNumber() == IAtom.H) {

}

Then the second step I would add a getElement() method that returns a primitive int (default 0) rather than the Integer.

[egonw]

I think either of the two options are good:

if (atom.getAtomicNumber() == IAtom.H) {

and

if (atom.getElement() == IAtom.H) {

The second is actually what I tried to make patches for at some point in the past, but it caused so many regressions back then. The idea behind all this was that String comparisons were expensive. Of course, this is may have quite changed in newer Java versions with internalized(?) Strings. The problem was that created every new IAtom would allocate a new String for the element, something already partially addresses in newer CDK versions too.

John, I'd be delighted with either way forward.

[johnmay]

IAtom.H would be an integer

[johnmay]

interface IAtom {
	
	public static final int Wildcard = 0;
	public static final int H        = 1;
	public static final int He       = 2;
	// ...
	public static final int B        = 5;
	public static final int C        = 6;
	public static final int N        = 7;

}

[egonw]

What about using a short? Worth it?

[johnmay]

Sure - could technically by a byte... but neither would save much. Java likes to work in Integers so most of the time types get upcast first.

[xperrylinn]

Hi guys, thanks for the context and discussion. Very helpful. I've managed to build the project and will open up a PR after making some progress.

Are there contribution guidelines that I should be aware of?

[egonw]

We do not have these clearly written up, but your patches will be peer reviewed by an experienced CDK developer. The replies may vary from time to time. Make simple, clean, clear patches help. Patches cannot be too small. In patches particularly, solve one thing. For example, if you run into a likely bug during your updates (that happens!), then put that bug fix in a separate patch. Also, whitespace clean up, leave that to John's scripts: putting that in your patch makes it harder to find the functional changes. Important, all patches must compile and not introduce regressions. So, before you submit a PR, make sure all tests still succeed.

General advice, start simple and small, and see how we reply. All feedback will not block the patch in the end, and is aimed at improving the code. (Generally, sometimes a patch is rejected, e.g. because a better solution comes up during the discussions.)

[xperrylinn]

Hi @johnmay and @egonw, do I need to request permissions to push my patch branch to the CDK repo?

I'm receiving the following message: remote: Permission to cdk/cdk.git denied to xperrylinn. after running push my new patch branch to origin, git push origin patch/phase_out_getSymbol_0

[johnmay]

Hi you should fork the project, push to a branch there and then open a PR

[xperrylinn]

Opened a PR here: cdk/cdk#636