#607 ShortestPathFingerprinter Unit Test Fixes

Accepted
closed
nobody
master
1
2015-04-16
2013-01-08
John May
No

These patches resolve the final failing unit test in the fingerprint module (except coverage). There was quite a bit which needed rejigging so I split it across many commit and it should be relatively clear what actually changed. Patches are on master+.

The combination of patches up till eb4a574 do not cause any test regressions. These were simply refactoring the code to allow cleaner integration of the actual bug fix.

The final commit (14d1c60) actually fixes the bug and will encode all equally length shortest paths (providing there isn't too many). This final commit causes two regressions as the molecules in those tests had multiple paths which weren't handled before.

These changes slash the computation time in half (110,000 ms -> 40,000 ms) and this could drop further as this fingerprinter uses the relavent rings. Turns out the new shortest path code is pretty useful for ring perception and I managed to write a relavent rings (union of all MCB/SSSR) which runs quicker then both SSSR.findSSSR and SSSR.findRelaventRings. Need to patch a few more things first before I submit that though.

Discussion

  • John May

    John May - 2013-01-08

    Commits start at: 5f2c66d

     
    • John May

      John May - 2013-01-08

      *start from.

      Don't pull the resolved conflicts :-).

       
  • John May

    John May - 2013-01-08
    • Description has changed:

    Diff:

    --- old
    +++ new
    @@ -4,5 +4,5 @@
    
     The final commit ([14d1c60](https://github.com/johnmay/cdk/commit/14d1c60764dc250250c46319ca96cdb45a6d564b)) actually fixes the bug and will encode all equally length shortest paths (providing there isn't too many). This final commit causes two regressions as the molecules in those tests had multiple paths which weren't handled before.
    
    -These changes slash the computation time in half (110,000 ms -> 40 ms) and this could drop further as this fingerprinter uses the relavent rings. Turns out the new shortest path code is pretty useful for ring perception and I managed to write a relavent rings (union of all MCB/SSSR) which runs quicker then both SSSR.findSSSR and SSSR.findRelaventRings. Need to patch a few more things first before I submit that though.
    +These changes slash the computation time in half (110,000 ms -> 40,000 ms) and this could drop further as this fingerprinter uses the relavent rings. Turns out the new shortest path code is pretty useful for ring perception and I managed to write a relavent rings (union of all MCB/SSSR) which runs quicker then both SSSR.findSSSR and SSSR.findRelaventRings. Need to patch a few more things first before I submit that though.
    
     
  • John May

    John May - 2013-03-26
    • status: open --> closed
    • milestone: Needs_Review --> Accepted
     
  • John May

    John May - 2013-03-26

    applied and pushed by Egon

     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks