Patch/20170504 #317

k-ujihara · 2017-05-05T03:06:43Z

Miscellaneous patches.

Or rename to 2,5-dimethylpyrrole

It may return null not throw exception.

Or delete the check.

I should not throw NoSuchElementException.

egonw · 2017-05-05T06:30:51Z

Dear Kazuya, first, welcome!

Oh, wow! Wonderful! There are some patches that should be applied with high priority, but it also has patches I need to look more close too.

@kazuyaujihara, a general would be for me to ask if you could indicate (where needed, and some patches are evident), what is being fixed? For example, what problem is fixed by commit c004679 . Also, we have as preference to have unit tests that first demonstrate the problem, e.g. because the existing code calculates the wrong answer. That makes it easier for the reviewer to verify the code, as we do no always manage to get the original author to look at the patch.

@johnmay, @kazuyaujihara, may I suggest we cherry-pick the easy ones (e.g. the one about the readers), and then we can review the remaining patches in more detail?

E.g. I think these patches are good to go:

And these ones probably too:

@johnmay, what do you think? Shall I cherry-pick these, and so that this PR can then be rebased?

k-ujihara · 2017-05-05T10:58:36Z

Dear Egon, thank you for accepting me. Because these patches are logically found on my way to port CDK to C#, I do not have any unit tests yet. So I will explain each of them tentatively. f335355 <f335355> --- The file name is 2,5-diMe-furan but the content is 2,5-diMe-pyrrole. Either of two should be corrected. fe2e31f <fe2e31f> --- There are the resemble codes in Rearrangement...Reaction classes, but only RearrangementRadicalReaction class is different. Need a unit test. 5de731a <5de731a> --- Because IsotopePatternRule.setParameters(Object[] params) accepts List<double[]> as params[0], getParameters()[0] is expected to return List<double[]>. d6ca462 <d6ca462>, 0c0bb18 <0c0bb18>, a22d04f <a22d04f>, a3af9ce <a3af9ce> --- Apparently, they are typo or typical mistakes. Need unit tests. 5687f86 <5687f86> --- This line never returns null and null is returned at last line on the proper condition. a66b51f <a66b51f> --- I believe logical And/Or should be used in this context. d759659 <d759659> --- I don't know there are other meaning or not. c004679 <c004679> --- I expect dblEqual(double v1, double v2) function is designed as nearly equal function. But it does not work correctly when v1 or v2 are negative. All current unit tests pass v1 == v2 to dblEqual. Need a unit test? 52a2279 <52a2279> --- My image of NoSuchElementException class is not used on this context. 2017-05-05 15:30 GMT+09:00 Egon Willighagen <notifications@github.com>:

…

Dear Kazuya, first, welcome! Oh, wow! Wonderful! There are some patches that should be applied with high priority, but it also has patches I need to look more close too. @kazuyaujihara <https://github.com/kazuyaujihara>, a general would be for me to ask if you could indicate (where needed, and some patches are evident), *what* is being fixed? For example, what problem is fixed by commit c004679 <c004679> . Also, we have as preference to have unit tests that first demonstrate the problem, e.g. because the existing code calculates the wrong answer. That makes it easier for the reviewer to verify the code, as we do no always manage to get the original author to look at the patch. @johnmay <https://github.com/johnmay>, @kazuyaujihara <https://github.com/kazuyaujihara>, may I suggest we cherry-pick the easy ones (e.g. the one about the readers), and then we can review the remaining patches in more detail? E.g. I think these patches are good to go: - dc3ee76 <dc3ee76> - 50a6127 <50a6127> - be89e66 <be89e66> - 59054f1 <59054f1> - e5df9ff <e5df9ff> And these ones probably too: - bd67561 <bd67561> - d6ca462 <d6ca462> @johnmay <https://github.com/johnmay>, what do you think? Shall I cherry-pick these, and so that this PR can then be rebased? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#317 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ARg5EI8FviL0kUfVCSweqULbhStPNxETks5r2sIcgaJpZM4NRckt> .

johnmay · 2017-05-05T11:07:55Z

base/core/src/main/java/org/openscience/cdk/atomtype/CDKAtomTypeMatcher.java

@@ -1016,7 +1017,7 @@ private boolean isSingleHeteroAtom(IAtom atom, IAtomContainer container) {

    }

-    private boolean isRingAtom(IAtom atom, IAtomContainer atomContainer, RingSearch searcher) {
+    private boolean isRingAtom(IAtom atom, IAtomContainer atomContainer, RingSearch searcher) throws NoSuchAtomException {


Nope!!!! I'm actually about the change the API to try and make NoSuchAtom an unchecked exception but please watch out for breaking the API.

johnmay · 2017-05-05T11:13:15Z

...cefield/src/main/java/org/openscience/cdk/modeling/builder3d/MMFF94BasedAtomTypePattern.java

@@ -163,7 +163,7 @@ private void createPattern() {
        //cdk.bug 3515122 fixed
        atomTypePatterns.add(Pattern.compile("N-[1-3];[CHN]{1,3}.{1}+[A-Z]{0,3}+[,]?+=O[CNXO].*+"));
        //NC=0 amid
-        atomTypePatterns.add(Pattern.compile("N-1-2];[CH]{1}+=S[(].*+"));
+        atomTypePatterns.add(Pattern.compile("N-[1-2];[CH]{1}+=S[(].*+"));


Maybe... but this class should be deprecated and has been replaced by Mmff. If it's not deprecated I'll get on it.

johnmay · 2017-05-05T11:14:28Z

Patches look great thanks for tracking them done. Yes please cherry pick @egonw - am okay with the ones you've selected so far. Others are also okay except for the NoSuchElementException (see comment), the cdk-reaction one's are tricky.

johnmay

Ignore - approve

johnmay · 2017-05-05T11:17:01Z

Ignore that approve... I though I could do it per commit.

johnmay · 2017-05-05T11:26:09Z

@egonw easier to list the ones that need more looking into:

fe2e31f - cdk-reaction need in depth look
a22d04f - cdk-reaction need in depth look
5de731a - okay to my eye by @egonw you'll no more.
c004679 - need to look closer, maybe check with Alex Clark
52a2279 - API Change, am considering fixing this ATM on my local branch (NoSuchAtom should be unchecked)

Others that are okay and why

f335355 fine, see filename
0c0bb18 fine, this was my bad :-)
5687f86 fine, Map<IReaction,Integer>
a66b51f fine, no change in meaning but more correct for logical conditions
a3af9ce fine, my bad again
e5df9ff fine - duplicate code
d759659 fine - duplicate code
59054f1 fine

by accident

johnmay · 2017-05-05T20:13:17Z

Thanks again.

I had a careful look at the ones I was unsure on and agree the patches are correct and merged all but the last one into master. I'll redo the NoSuchAtomException once #318 is applied (or feel free to do it yourself). Out of interest how did you find these errors, lots of them seem very specific?

k-ujihara · 2017-05-06T01:31:46Z

@johnmay
I reviewed several classes to learn cdk and convert to C#. Few classes are refactored on C# code. Most of errors are found during it.
Thanks for a great library.

johnmay · 2017-05-06T07:24:13Z

Well thanks again, very nice finds, just reviewing the other pull request now. Did you use IKVM for C# or port the code? Might be something we can host.

k-ujihara · 2017-05-09T03:48:52Z

All codes are written in C#. The repository is the following.
https://github.com/kazuyaujihara/NCDK
I am happy to host it on the cdk.

k-ujihara added 18 commits May 5, 2017 10:15

Correct compound name

dc3ee76

Fix structure data

f335355

Or rename to 2,5-dimethylpyrrole

Fix RearrangementRadialReaction

fe2e31f

IsotopePatternRule.getParametes() returns List<double[]>

5de731a

Fix NSO(50) pattern

d6ca462

Missing break

0c0bb18

Fix some setReader(Reader reader).

50a6127

Fix RDFProtonDescriptor_GDR

a22d04f

Fix MolecularFormulaManipulator.getMolecularFormula("", ...)

be89e66

It may return null not throw exception.

Inproper parameter check

5687f86

Or delete the check.

Fix disjoint tests

bd67561

& --> && and | --> ||

a66b51f

Remove exra 0

a3af9ce

Remove extra line

e5df9ff

Remove extra check

d759659

Fix dblEqual

c004679

Fix type of l-value for getFormalCharge()

59054f1

RingSearch.cyclic(IAtom atom) throws NoSuchAtomException

52a2279

I should not throw NoSuchElementException.

johnmay reviewed May 5, 2017

View reviewed changes

johnmay previously approved these changes May 5, 2017

View reviewed changes

johnmay reviewed May 5, 2017

View reviewed changes

johnmay closed this May 5, 2017

k-ujihara deleted the patch/20170504 branch May 28, 2017 02:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Patch/20170504 #317

Patch/20170504 #317

k-ujihara commented May 5, 2017

egonw commented May 5, 2017

k-ujihara commented May 5, 2017 via email

johnmay May 5, 2017

johnmay May 5, 2017

johnmay commented May 5, 2017

johnmay left a comment

johnmay commented May 5, 2017

johnmay commented May 5, 2017

johnmay commented May 5, 2017

k-ujihara commented May 6, 2017

johnmay commented May 6, 2017 •

edited

k-ujihara commented May 9, 2017

Patch/20170504 #317

Patch/20170504 #317

Conversation

k-ujihara commented May 5, 2017

egonw commented May 5, 2017

k-ujihara commented May 5, 2017 via email

johnmay May 5, 2017

Choose a reason for hiding this comment

johnmay May 5, 2017

Choose a reason for hiding this comment

johnmay commented May 5, 2017

johnmay left a comment

Choose a reason for hiding this comment

johnmay commented May 5, 2017

johnmay commented May 5, 2017

johnmay commented May 5, 2017

k-ujihara commented May 6, 2017

johnmay commented May 6, 2017 • edited

k-ujihara commented May 9, 2017

johnmay commented May 6, 2017 •

edited