Camelia, the Perl 6 bug

IRC log for #cdk, 2009-03-11

| Channels | #cdk index | Today | | Search | Google Search | Plain-Text | summary

All times shown according to UTC.

Time Nick Message
05:12 jbrefort joined #cdk
06:01 egonw joined #cdk
06:58 carsten joined #cdk
07:25 CIA-62 cdk: egonw * r14417 /cdk/branches/cdk-1.2.x/src/ (2 files in 2 dirs): (log message trimmed)
07:25 CIA-62 cdk: It is not clear if the below patches are bug fixes, neither should @cdk.bug annotation be removed from unit tests.
07:25 CIA-62 cdk: Miguel, please use the patch system, as cdk1.2.x is in deep freeze, and all new functionality needs to be reviewed by
07:25 CIA-62 cdk: two reviewers (which should not really be a problem for the patches, but it just needs to happen)...
07:25 CIA-62 cdk: Bug fixing can generally go in directly and is reviewed by me via the commits mailing list.
07:25 CIA-62 cdk: Revert "Set value 1 when contains only the signal for specify the charge."
07:25 CIA-62 cdk:  This reverts commit 94613ba13effbf36d21c667bbcfc3f7b9701b183.
07:32 Gpox joined #cdk
08:13 rojasm joined #cdk
08:41 egonw joined #cdk
08:41 rojasm moin egonw
08:41 rojasm I read your email
08:42 rojasm Can I submit a patch with eclipse?
08:44 egonw hi rojasm
08:44 egonw not sure
08:44 egonw never tried
08:44 rojasm I try to submit the patch for eclipse
08:44 egonw otherwise, run a 'git diff' or 'svn diff' ...
08:44 egonw and upload the output to the patch tracker
09:20 shk3 joined #cdk
09:23 jonalv joined #cdk
10:50 jbrefort joined #cdk
12:33 tim joined #cdk
12:39 jonalv joined #cdk
12:42 CIA-62 cdk: egonw * r14418 /cdk/branches/cdk-1.2.x/src/main/org/open​science/cdk/tools/SaturationChecker.java: Removed redundant throws clauses (fixes #2665320)
13:32 CIA-62 cdk: rajarshi * r14419 /cdk/branches/cdk-1.2.x/src/ (3 files in 2 dirs):
13:32 CIA-62 cdk: Updated code so that hard coded H's are properly added to a pseudoatom. However,
13:32 CIA-62 cdk: the charge is still not associated with this type of atom. Furthermore,
13:32 CIA-62 cdk: roundtripping does not work as the output is still a single * with no mass
13:32 CIA-62 cdk: number or H count etc. So bug 2596061 still remains open
13:36 CIA-62 cdk: rajarshi * r14420 /cdk/branches/cdk-1.2.x/src/test/org/opensc​ience/cdk/smiles/SmilesGeneratorTest.java: Fixed the round tripping test to actually do the test (doh!)
13:41 CIA-62 cdk: rajarshi * r14421 /cdk/branches/cdk-1.2.x/src/main/org/open​science/cdk/smiles/SmilesGenerator.java:
13:41 CIA-62 cdk: Updated code so that if an atom is a PSeudoAtom we don't just output a * and be
13:41 CIA-62 cdk: done with it. Rather, we specifically set the symbol to * but then treat it as
13:41 CIA-62 cdk: any other atom. Address roundtripping for bug 2596061, but isn't completely
13:41 CIA-62 cdk: fixed yet, since H count and charge doesn't seem to be properly output.
14:54 CIA-62 cdk: rajarshi * r14422 /cdk/branches/cdk-1.2.x/src/main/org/open​science/cdk/smiles/SmilesGenerator.java: Updated code so that it adds the implicit H count for a pseudoatom
15:02 CIA-62 cdk: rajarshi * r14423 /cdk/branches/cdk-1.2.x/src/main/org/open​science/cdk/smiles/SmilesGenerator.java: Updated code so that if mass number for a pseudo atom is null, return an empty string
15:19 CIA-62 cdk: egonw * r14424 /cdk/branches/cdk-1.2.x/src/ (3 files in 3 dirs): Added perception of I.3 as in iodosobenzene
15:21 CIA-62 cdk: egonw * r14425 /cdk/branches/cdk-1.2.x/src/ (3 files in 3 dirs): Added perception of I.5 as in iodoxybenzene
15:23 CIA-62 cdk: egonw * r14426 /cdk/branches/cdk-1.2.x/src/ (3 files in 3 dirs): Added perception of two-coordinate, two pi-bond S atom type (like S.oxide, but then more general)
15:24 CIA-62 cdk: shk3 * r14427 /cdk/branches/jchempaint-primar​y/src/main/org/openscience/cdk/ (6 files in 5 dirs): added merge of atoms while moving
15:24 CIA-62 cdk: egonw * r14428 /cdk/branches/cdk-1.2.x/src/ (3 files in 3 dirs): Added perception of four-coordinate, single-bond-only S atom type
15:34 CIA-62 cdk: rajarshi * r14429 /cdk/branches/cdk-1.2.x/src/ (2 files in 2 dirs): Updated code so that hard coded H's are parsed properly whether they are followed bby a digit or not. Fixes failing tests and added another unit test to check specifically for edge cases
15:34 CIA-62 cdk: egonw * r14430 /cdk/branches/cdk-1.2.x/src/ (2 files in 2 dirs): Loosend scope of S.inyl. No longer restricted to oxygen and nitrogen species
15:35 egonw +        // This is a horrible kludge :( We need a JavaCC parser!
15:36 egonw :)
15:36 egonw rajarshi++
15:36 egonw yes, I think we do
15:37 CIA-62 cdk: rajarshi * r14431 /cdk/branches/cdk-1.2.x/src/test/org/openscience​/cdk/smiles/smarts/parser/SMARTSSearchTest.java: Added a test for the recursive lactam pattern that works, when using a negation operator
15:38 rajarshi joined #cdk
15:38 rajarshi hi
15:38 egonw hej rajarshi!
15:38 zarah ni hao rajarshi
15:38 * egonw is catching the bus in 12 minutes
15:39 egonw it's amazing how buggy the CDK 1.0 SMILES parser is...
15:39 rajarshi hmmm
15:39 egonw I second your JavaCC suggestion, see email
15:39 rajarshi saw that
15:39 rajarshi may Ishould try and bribe Dazhi :)
15:39 egonw would be fun to run the current SMILES test suite on the old SMILES parser :)
15:40 rajarshi indeed
15:42 egonw do you know the state of the 2-letter elements in FP bug report?
15:42 rajarshi yes
15:42 rajarshi it's a trivial fix, but the 3d builder fp's need to be rebuilt
15:43 rajarshi I'm afraid stefan might fly across the atlantic and try to kill me :)
15:43 rajarshi (if I change them again)
15:43 egonw yes, let's wait after the ACS :)
15:43 egonw seriously...
15:43 egonw I think he wrote up a wiki describing how to do it
15:43 rajarshi well I did add the ant target to regenerate the fp gz files
15:43 rajarshi so at least that part is easy
15:44 egonw ah, right
15:44 egonw so, what's holding us back?
15:44 rajarshi i don't what (if anything) needs to be done after that, thats why I'm holding on to the fix
15:45 egonw I think the concensus is to fix just the 2-letter stuff for the FP
15:45 egonw and leave the rest until after 1.2
15:45 egonw of that report
15:45 rajarshi ok, I can go ahead and do that - actually I think that was the only thing left
15:45 rajarshi I went ahead and rewrote the path finding code to be mor eefficient
15:45 egonw cool
15:46 egonw btw, what's your catch on this one:
15:46 egonw http://sourceforge.net/tracker2/?func=detail&amp​;aid=2514200&group_id=20024&atid=120024
15:46 zarah egonw's link is also http://tinyurl.com/csy9u2
15:46 egonw since your recent patches sound related
15:46 rajarshi one sec
15:47 rajarshi stupid SF authentication
15:47 egonw indeed
15:47 egonw at least I now know why I have to log in all the time
15:47 rajarshi oh, this just got fixed
15:47 egonw changin IP makes the cookies go bad
15:47 rajarshi i was just about to close it :)
15:47 egonw oh, excellent
15:47 egonw then we are good to go with 1.2.0 :)
15:48 rajarshi looks like it
15:48 rajarshi still have to see why pseudoatom smiles don't consider charges
15:48 rajarshi andrews bug report closing is pendig on that
15:49 egonw do you want to explore that before 1.2.0 ?
15:49 egonw weird actually
15:49 rajarshi yes, looking at it now
15:49 egonw as formal charges should be supported for PseudoAtoms now...
15:49 egonw OK, going to catch my bus now...
15:49 egonw will be online tonite
15:49 egonw for the magic 1.2.0 release
15:49 rajarshi btw, the fp report still needs work. I think by handling the 2-letter symbols properly we can go with 1.2. The remaining issues are performance related 9some of which have been addressed, but others can wait)
15:49 egonw hoping that you found the bug by then
15:49 rajarshi ok, see you
15:49 rajarshi :)
15:50 egonw yes, 1.2.0 is not critical regarding performance...
15:50 egonw would be good to get this out, and drop SVN
15:50 egonw so much looking forward to that
15:50 egonw OK, cu later!
15:50 rajarshi i agree
15:53 CIA-62 cdk: rajarshi * r14432 /cdk/branches/cdk-1.2.x/src/test/org/​openscience/cdk/PseudoAtomTest.java: Added simple unit test tocheck that charges get set properly
16:04 CIA-62 cdk: rajarshi * r14433 /cdk/branches/cdk-1.2.x/src/test/org/open​science/cdk/smiles/SmilesParserTest.java: Added bug annotation
16:09 CIA-62 cdk: rajarshi * r14434 /cdk/branches/cdk-1.2.x/src/main/org/op​enscience/cdk/smiles/SmilesParser.java: Updated code so that after the molecule is parsed, we do atom typing but only set the *unset* properties from the atom type and leave the properties that came in from the SMILES as is. Fixes bug 2596061
16:18 CIA-62 cdk: shk3 * r14435 /jchempaint/trunk/src/main/org/openscience/ (3 files in 3 dirs): added the merge while moving
16:20 CIA-62 cdk: shk3 * r14436 /jchempaint/trunk/src/main/org/openscience/jch​empaint/undoredo/SwingMergeMoleculesEdit.java: added the merge while moving
16:22 CIA-62 cdk: rajarshi * r14437 /cdk/branches/cdk-1.2.x/src/main/org/op​enscience/cdk/smiles/SmilesParser.java: reverted to origjnal form of atom configuration - the problem is in how we do configuration for pseudo atoms rather than smiles parsing which is now fine
16:28 CIA-62 cdk: rajarshi * r14438 /cdk/branches/cdk-1.2.x/src/main/org/openscience​/cdk/tools/manipulator/AtomTypeManipulator.java:
16:28 CIA-62 cdk: Dont' configure the atom if it is a PseudoAtom
16:28 CIA-62 cdk: configuring aotm type information is not really valid for pseudo atoms - first
16:28 CIA-62 cdk: because they basically have no type information and second because they may have
16:28 CIA-62 cdk: information associated with them from another context, which should not be
16:28 CIA-62 cdk: overwritten. So we only do the stuff below if we have a non pseudoatom
16:28 CIA-62 cdk: a side effect of this is that it is probably not valid to get the atom type of a peudo atom. I think this is OK, since you can always check whether an atom is a pseudo atom without looking at its atom type
16:28 CIA-62 cdk: rajarshi * r14439 /cdk/branches/cdk-1.2.x/src/test/org/opensc​ience/cdk/smiles/SmilesGeneratorTest.java: clenaed the test to remove stdout output
17:27 CIA-62 cdk: shk3 * r14440 /cdk/branches/jchempaint-primary/src/​main/org/openscience/cdk/controller/ (ControllerHub.java IChemModelRelay.java MoveModule.java): atoms no longer get drawn overlapping
18:37 egonw joined #cdk
18:39 egonw joined #cdk
19:18 rajarshi hi egonw
19:18 egonw hej
19:18 zarah egonw: You have new messages. Write '/msg zarah @messages' to read them.
19:18 egonw @msg
19:18 zarah egonw: 3 h 3 m 1 s ago, masak said did you tell jonalv that he should rebase his upstream? I can't imagine you did, but he seems very insistent.
19:22 rajarshi is it OK if I move PeriodicTable to standard?
19:22 egonw where does it come from?
19:22 rajarshi extra
19:22 egonw does it use only interfaces?
19:23 egonw is it fully unit tested?
19:23 egonw PMD error free?
19:23 egonw no DocCheck errors?
19:23 rajarshi unit tested
19:23 rajarshi doesn't use interfaces (though some of the clases it uses don't have interfaces)
19:24 rajarshi hmm, PeriodicTableElement would also need to be moved though
19:24 rajarshi actually, forget about it for now
19:24 rajarshi aargh!
19:25 rajarshi if we want to fix the 2-element issue in fingerprinter, it will need access to PeriodicTable
19:25 egonw why?
19:25 rajarshi to look up atomic numbers
19:26 egonw Element symbol -> atomic number?
19:26 rajarshi yes
19:26 egonw maybe we should make a patch to Symbols...
19:27 egonw and make this a HashMap
19:27 rajarshi Symbols?
19:27 egonw that goes both ways...
19:27 egonw cdk.config.Symbols
19:27 egonw OK, hang on...
19:27 rajarshi aaah, didn't know about that
19:27 egonw let me fix a few things in your patches first :)
19:27 rajarshi what went wrong?
19:28 egonw you'll see :)
19:28 egonw nothing big
19:28 egonw or bug
19:28 rajarshi :)
19:30 CIA-62 cdk: rajarshi * r14441 /cdk/branches/cdk-1.2.x/src/main/org/opensc​ience/cdk/config/data/chemicalElements.xml: Added the R (indicating a pseudo atom) element with atomic number 0
19:30 rajarshi btw, the functionality in Symbols already exists in PeriodicTable
19:30 egonw yeah, but more isolated...
19:30 egonw and cleaner
19:30 egonw ummm...
19:30 egonw as in, more modular...
19:31 egonw single job
19:31 egonw PT does much more, not?
19:31 egonw it should reuse Symbols, I guess
19:31 rajarshi yes, it stores all relevent constant info wrt elements
19:31 rajarshi and reads from the XML file from BODR
19:31 rajarshi it seems it's better to locate all this in one class
19:31 rajarshi and since it uses a HashMap (at num -> sym) it's currently faster than symbols nyway
19:32 egonw why?
19:32 egonw Symbols uses an array
19:32 rajarshi oops, sorry
19:32 rajarshi right
19:32 egonw mmmm...
19:32 egonw I get 5 fails in PseudoAtomTest now...
19:32 egonw which I had not earlier...
19:33 egonw 3 on hydrogen count unit tests
19:33 egonw 2 on formal charge
19:33 rajarshi hold on
19:33 egonw just synched with svn
19:33 egonw and updated my eclipse workspace
19:33 rajarshi very strange
19:33 rajarshi let me look
19:34 rajarshi oh right
19:34 CIA-62 cdk: egonw * r14442 /cdk/branches/cdk-1.2.x/src/test/org/​openscience/cdk/PseudoAtomTest.java: Typo
19:34 rajarshi h count and charge were modified in PseudoAtom to not be set to 0 by default
19:34 rajarshi so that if these were specified they'd be included
19:34 rajarshi the tests assume 0's by default
19:35 egonw so, when you change PseudoAtom you did not update the unit tests?
19:35 rajarshi right, forgot
19:35 rajarshi one sec
19:35 rajarshi actually the problem is in AbstractPseudoAtomtest
19:35 CIA-62 cdk: egonw * r14443 /cdk/branches/cdk-1.2.x/src​/test/org/openscience/cdk/ (PseudoAtomTest.java interfaces/AbstractPseudoAtomTest.java): Moved test so that it is also run for DebugPseudoAtom and NNPseudoAtom
19:36 egonw yes likely
19:36 egonw because all unit tests for the IPseudoAtom interface are in that class...
19:36 egonw see also the commit I just made:
19:37 egonw 14443
19:37 rajarshi saw that
19:37 CIA-62 cdk: egonw * r14444 /cdk/branches/cdk-1.2.x/src/main/org/openscience​/cdk/tools/manipulator/AtomTypeManipulator.java: Use the interface instead of the implementation
19:40 CIA-62 cdk: rajarshi * r14445 /cdk/branches/cdk-1.2.x/src/main/or​g/openscience/cdk/PseudoAtom.java: fixed a comment
19:42 rajarshi is the deafult value of charge in an atom type object meant ot be 0 or null?
19:45 rajarshi hmm, looks like null in AtomType, so why does AbstractAtomTypeTest check for a zero?
19:46 egonw mom
19:46 CIA-62 cdk: rajarshi * r14446 /cdk/branches/cdk-1.2.x/src/test/org/openscien​ce/cdk/interfaces/AbstractPseudoAtomTest.java:
19:46 CIA-62 cdk: Updated tests to take into account that we can set H count and charges on a
19:46 CIA-62 cdk: pseudo atom if desired. Thus its default behavior is more like that of IAtom.
19:46 CIA-62 cdk: (Right now only stereo parity is set to 0 by default for a pseudo atom)
19:46 egonw check the constructors
19:48 egonw the constructors prefil some values...
19:48 egonw zero charge is a good default...
19:48 egonw at least chosen in the past
19:48 rajarshi ok, but AtomType sets it to null and AbstractAtomTypeTests coerces to IAtomType
19:48 egonw neutral atoms are more common...
19:48 egonw so when you say give me a "C"
19:48 egonw people expect a neutral carbon
19:49 rajarshi IAtom's default value is null though
19:49 egonw Atom, you mean?
19:49 rajarshi right
19:49 egonw IAtom does not have a value
19:50 rajarshi ? it takes the default from AtomType
19:50 egonw sure?
19:50 egonw I think the Atom constructors call the AtomType constructors
19:50 egonw which sets the formal charge to 0
19:50 rajarshi correct - formalCharge is never set in Atom
19:50 egonw right
19:50 rajarshi and AtomType sets it to UNSET (i.e., null)
19:50 egonw because it is an IAtomType property
19:50 rajarshi this was the agreed procedure
19:51 egonw where?
19:51 egonw I only see line 115
19:51 rajarshi AtomType.java line 80
19:51 egonw in AtomType.java
19:51 egonw ah...
19:51 egonw well, yes and no
19:52 egonw that defines the class defaults
19:52 egonw but the object defaults are defined by the constructors
19:52 rajarshi hmm, ok
19:52 egonw ic understand this confusion and it is a bit inconsistent, I guess...
19:53 egonw but consider that constructors tune what users want
19:53 rajarshi i thought it was agreed that anything not explicitly set was UNSET
19:53 egonw and do indeed not reflect any interface or so...
19:53 egonw in that respect
19:53 rajarshi i see
19:53 egonw neither do the AtomType.class default
19:53 egonw s
19:53 egonw oh yes...
19:53 egonw but that's something else...
19:54 egonw AFAIK the formal charge *is* deliberately set to zero
19:54 egonw not as value for UNSET
19:54 egonw but
19:54 egonw to make the atom be neutral
19:54 egonw and not have an unknown charge
19:54 rajarshi oh ok
19:56 rajarshi ok, all PseudoAtomTests are now fine
19:56 CIA-62 cdk: rajarshi * r14447 /cdk/branches/cdk-1.2.x/src/ (2 files in 2 dirs): Updated PseudoAtom so that it does not provide defaults for h count and foprmal charge but rather uses whatvever its superclass uses (which happens to be null and 0). Also updated test case
19:57 CIA-62 cdk: rajarshi * r14448 /cdk/branches/cdk-1.2.x/src/main/o​rg/openscience/cdk/AtomType.java: Updated Javadoc
20:00 egonw that last commit is not the correct way around...
20:00 egonw the constructor may choose to...
20:00 rajarshi how so?
20:00 egonw but a subclass may change the constructor behavior
20:00 egonw actually...
20:01 egonw the third constructor copies the value from the past thingy...
20:01 egonw thingy = IElement
20:01 egonw line 149
20:01 rajarshi oh right, forgot to specify which constructor
20:01 egonw so, better to have the constructor say what it does
20:01 egonw instead of adding that to the JavaDoc of the class field...
20:01 rajarshi AtomType(String) does say that
20:02 egonw right
20:02 rajarshi but I wanted to make it explicit in the field doc as well
20:02 egonw which is enough, I guess
20:02 egonw but there it is not true
20:02 rajarshi yes, updating it
20:02 egonw but I understand that the comment that it default to null by RFC #6 is not quite accurate, and confusing at least
20:02 egonw well, it is accurate...
20:02 egonw but still confusing
20:03 CIA-62 cdk: rajarshi * r14449 /cdk/branches/cdk-1.2.x/src/main/o​rg/openscience/cdk/AtomType.java: Made the javadoc more specific
20:07 CIA-62 cdk: rajarshi * r14450 /cdk/branches/cdk-1.2.x/src/main/org/op​enscience/cdk/tools/PeriodicTable.java: Updated code to satisfy PMD
20:07 egonw Shall I convert Symbols to use a HashMap and go both ways?
20:08 rajarshi OK - but I still think we should consolidate this into PeriodicTable, because if you do that change it's functionality is identical to PeriodicTable
20:08 rajarshi at lest it will let fingerprinter get the atomic numbers
20:08 rajarshi for now
20:09 egonw but that class cannot go into standard
20:09 egonw if it uses PeriodicTableElement
20:09 egonw which depends on the data module
20:09 rajarshi right
20:09 rajarshi after 1.2 I would really like to overhaul the PT stuff
20:09 egonw yes, sounds like a good idea
20:10 rajarshi right now PeriodicTableElement is simply used internally for reading the XML files and loading PeriodicTable
20:10 rajarshi so it really should not be visible to the user
20:10 rajarshi so yes, if you can convert Symbols to a HashMap I can fix the fingerprinter
20:10 egonw ok
20:20 rajarshi btw, if you update Symbols, can you put in 'R' with an atomic number of 0
20:20 egonw ok, just in time...
20:20 egonw no, I rather not
20:20 egonw R is undefined I would say
20:21 egonw can be anything
20:21 egonw likely a fragment
20:21 egonw why set to 0?
20:21 rajarshi because it is a pseudoatom
20:21 rajarshi this is the convention in OEChem (as Andrew pointed out)
20:21 egonw bad convention
20:22 rajarshi why? an object with at num  = 0 will have to be considered in more detail
20:22 rajarshi so in the end it's just an indicator
20:22 egonw num = UNSET too
20:22 rajarshi as an example: one of the fingerprinter tests looks at a molecule with an R as an element
20:23 rajarshi while I can check whether it's a pseudo atom, why not just look up the R symbol (since all PseudoAtom instances have R as the symbol)
20:23 egonw if the FP wants to support R, then that code should decide how to handle it
20:23 egonw e.g. use num=0
20:23 egonw yeah, I guess they should really have UNSET as symbol...
20:23 egonw but that was not possible at the time, I guess
20:25 rajarshi it seems that both are equivalent
20:26 rajarshi it boils down to : do a lookup or do an instanceof check
20:27 egonw instanceof
20:27 egonw it makes it more clear in the code that IPseudoAtom are acceptable input
20:27 egonw and more clear how they are handled...
20:27 rajarshi that's true
20:28 egonw also, what if someone actually subclasses PseudoAtom and uses a different symbol?
20:28 rajarshi aah, good point
20:28 egonw or just overwrites the default... or, is "R" fixed?
20:28 egonw not sure...
20:28 rajarshi well the constructor for PseudoAtom sets symbol to R, but a subclass can always change it if desired
20:29 egonw I love code review :)
20:29 egonw crap... Karin comes home from dinner downtown
20:29 egonw and I ahven't done the dishes yet...
20:29 egonw panic mode
20:29 rajarshi :)
20:38 CIA-62 cdk: egonw * r14451 /cdk/branches/cdk-1.2.x/src/ (2 files in 2 dirs): Added a convenience method to the an elements atomic number from it's symbol
20:40 rajarshi thanks
20:42 egonw I am a bit worried about the number of failing tests...
20:42 egonw I count this here about about 100
20:43 egonw ok, that includes 10 for the inchi lib not working on ubuntu jaunty
20:44 egonw but I think this is higher than with 1.1.5 and 1.1.6
20:44 egonw mom
20:44 egonw ah, no
20:44 egonw 1.1.5 had 90 fails...
20:44 egonw so, that matches actually...
20:44 rajarshi ok
20:45 egonw but since there are a few fixes...
20:45 egonw there should also be a few new regressions
20:45 rajarshi hopefully not (wishful thinking?)
20:45 egonw yes, think so...
20:45 egonw I do believe in the overall conservation of unit tests...
20:49 CIA-62 cdk: rajarshi * r14452 /cdk/branches/cdk-1.2.x/src/ (2 files in 2 dirs): (log message trimmed)
20:49 CIA-62 cdk: Updated fingerprinter to convert all atom symbols (1-letter and 2-letter) by
20:49 CIA-62 cdk: using their atomic number as a char, as described Andrew in bug 2581724. Note
20:49 CIA-62 cdk: that the fingerprint now ignores pseudo atoms. One reason for ingoring thisis
20:49 rajarshi that makes sense
20:49 CIA-62 cdk: that if we consider 0 as the aotmic number for a pseudo atom, it's ASCII value
20:49 CIA-62 cdk: becomes null - in the hascode function it appears that these contribute nothing
20:49 CIA-62 cdk: to the hashcode for a given string. So ignoring pseudo atoms seems to make
20:49 egonw got a plot on the performance?
20:50 egonw besides fixing the 2-letter cases...
20:50 egonw does it indeed speed up as Andrew suggested?
20:50 rajarshi I had already sent a mail looking at performance. with the HashMap lookup from PeriodicTable we take 0.9s longer
20:50 rajarshi this fix wasn't for speedup - but for completeness
20:50 egonw ah, indeed...
20:50 egonw it still uses char
20:51 egonw I see a problem with this patch
20:51 egonw it will cause a NPE
20:51 rajarshi as Andrew described in the thread, atomic number could ideally be intialized in the Element constructur,  but we decided to leave that for post 1.2
20:51 egonw for bad symbols
20:51 rajarshi aargh, right
20:51 egonw for which Symbols.getAtomicNumber() returns null
20:51 egonw I would also suggest to do support pseudo atoms
20:51 egonw and use as char
20:52 egonw the first after those used for existing symbols
20:52 egonw that is...
20:52 egonw Symbols.byAtomicNumber.length+1 or so
20:52 rajarshi aah, ok
20:52 egonw you could use that for both IPseudoElement and new Atom("Crap")
20:55 egonw I think I'll do the 1.2.0 tomorrow
20:55 rajarshi ok, I need to update the builder 3d fp's as well
20:55 egonw right
20:59 CIA-62 cdk: rajarshi * r14453 /cdk/branches/cdk-1.2.x/src/main/org/opens​cience/cdk/fingerprint/Fingerprinter.java: Updted fingerprint to consider pseudo atoms, as well as handle atoms for which malformed symbols (i.e., not in the periodic table) occur. This ensures that whatever is in the molecules gets considered in the fingerprint
21:02 egonw regarding the TODO...
21:02 egonw I will propose later to make IElement.atomicNumber a Short instead of an Integer
21:03 egonw 255 elements should be enough for everyone :)
21:03 rajarshi sounds good (atr least in this universe :)
21:03 egonw should help a bit for really large protein structures
21:03 egonw how much remains to be calculated
21:03 egonw anyway...
21:03 egonw going offline now for those dishes
21:04 egonw and to bad after that
21:04 egonw bed
21:04 egonw looking forward to your update of 3D index FP update
21:04 egonw and will release 1.2.0 tomorrow then
21:04 egonw which will bring us a Git world likely too...
21:05 rajarshi down to 8 bugs, with
21:05 rajarshi 1 level 9 bug :)
21:05 egonw which one?
21:05 egonw oh, the SMILES one?
21:05 rajarshi https://sourceforge.net/tracker2/?func=detail&am​p;aid=1014344&group_id=20024&atid=120024
21:05 egonw yes...
21:05 rajarshi yes
21:05 egonw some thing with making them unique
21:05 egonw have not been able to pinpoint why the unique numbering of atoms is not working...
21:06 egonw so, if people have unqiue SMILES index, they will have to update that too
21:06 egonw it actually sounds like a bug in the Morgan numbering tool...
21:06 egonw but could not find the source
21:06 egonw (of the problem)
21:06 rajarshi my analsys indicates that it's a problem with overwriting fields somewhere
21:07 egonw btw, the git repos on SF for the CDK is shadowing cdk1.2.x
21:07 egonw and is uptodate
21:07 egonw except for your last patch
21:07 rajarshi which one/
21:07 rajarshi i'm going to wait for a merge back into trunk and then start from there
21:08 egonw hang on...
21:08 egonw trunk only has some 10-ish commits
21:08 egonw I'll port those manually to git later...
21:08 egonw ... this week
21:08 rajarshi so does that mean the git repo shadowing 1.2 will become trunk?
21:08 egonw after I have figured out how to make tags on SF
21:08 egonw yes
21:09 egonw as trunk is only very little more
21:09 rajarshi i thought that somethings went into trunk sometime back but not into 1.2?
21:09 egonw ok, git@sf now has your last patch too
21:09 rajarshi or were there any syncs in between?
21:09 egonw yes
21:09 rajarshi oh ok
21:09 egonw but rebasing does not work in the git-svn world
21:09 egonw tried that
21:10 egonw but svn view on history is too different (only linear) wrt to git history...
21:10 egonw and git-svn does not understand what patches in trunk are after the branch...
21:10 rajarshi aah
21:10 egonw and so, which patches from the branch to merge back in when doing the rebase...
21:10 rajarshi well all the more reason for a complete move to git
21:10 egonw messedup
21:10 egonw indeed
21:10 rajarshi i love git branching :)
21:11 egonw same here
21:11 egonw OK, going offline now
21:11 egonw bye!
21:11 rajarshi ok, good night
21:11 egonw thanx for your fixes!
21:11 rajarshi np :)
21:17 CIA-62 cdk: rajarshi * r14454 /cdk/branches/cdk-1.2.x/src/main/org/​openscience/cdk/modeling/builder3d/da​ta/ringTemplateFingerprints.txt.gz: Updated builder 3D fingerprints due to the change in the fingerprint code
21:24 CIA-62 cdk: rajarshi * r14455 /cdk/branches/cdk-1.2.x/src/test/org/openscience​/cdk/smiles/smarts/parser/SMARTSSearchTest.java: Added some more test cases for the c-c SMARTS bug
21:39 CIA-62 cdk: rajarshi * r14456 /cdk/branches/cdk-1.2.x/src/main/o​rg/openscience/cdk/isomorphism/mat​chers/smarts/OrderQueryBond.java:
21:39 CIA-62 cdk: Updated match condition so that if the query was an explicit single bond, we
21:39 CIA-62 cdk: make sure that it matches if the target bond is single and aliphatic, as noted
21:39 CIA-62 cdk: in the day light spec. For double and triple, we do not check for aliphatic /
21:40 CIA-62 cdk: aromatic
21:40 CIA-62 cdk: rajarshi * r14457 /cdk/branches/cdk-1.2.x/src/test/org/openscience​/cdk/smiles/smarts/parser/SMARTSSearchTest.java: Fixed a typo in the unit test
21:55 CIA-62 cdk: rajarshi * r14458 /cdk/branches/cdk-1.2.x/src/main/o​rg/openscience/cdk/isomorphism/mat​chers/smarts/OrderQueryBond.java: reverted to original form since it seems to cause more problems
22:02 bag joined #cdk
22:19 CIA-62 cdk: rajarshi * r14459 /cdk/branches/cdk-1.2.x/src/test/org/openscience​/cdk/smiles/smarts/parser/SMARTSSearchTest.java: Added another test case for the unpecified bond query
22:57 CIA-62 cdk: rajarshi * r14460 /cdk/branches/cdk-1.2.x/src/ (4 files in 3 dirs): (log message trimmed)
22:57 CIA-62 cdk: Updated code to properly handle the case where a bond is not explicitly
22:57 CIA-62 cdk: specified. Daylight indicates that in such a case a bond can be aromatic or
22:57 CIA-62 cdk: single. We now provide a specific class to handle this
22:57 CIA-62 cdk: (AromaticOrSingleQueryBond) since the previous code was choosing either aromatic
22:57 CIA-62 cdk: or single based on the atoms. But this fails in the case of a molecule like
22:57 CIA-62 cdk: c1ccccc1c2ccccc2 where there are two aromatic atoms joined by a single bond.

| Channels | #cdk index | Today | | Search | Google Search | Plain-Text | summary