SourceForge has been redesigned. Learn more.
Close

#651 AllCycles/AllRingsFinder

Accepted
closed
nobody
master
1
2013-08-01
2013-06-28
John May
No

Okay split this in to two braches...

  • [feature/all-cycles] is the new implementation (working on a 'graph') - I also modified the existing cycles so they all give the same representation. That is they start and end with the same vertex {0, 1, 2, 3, 4, 5, 0} = 6 member ring. A couple of tests had to be changed but it's better to be consistent. first commit - a26c853
  • [feature/improved-all-rings-finder] use the new implementation to replace the AllRingsFinder version. I've kept the API the same (except of an Integer to int conversion but that is minimal). I was going to wait and do all the ring sets (i.e. for the other cycle perception) at once but decided this was quite important. The way the conversion is done might changed but that's class private so we can do that it future. first commit - 1e7d94f

Oh and needs the guava patch.

Discussion

  • John May

    John May - 2013-06-28

    Oh and the timings are still good, there's a bit of lag with the ring conversion but it's minimal and still offers a massive improvement.

     
  • Egon Willighagen

    Thanx, I'll try to review this asap. (but don't let that stop others of reviewing it before me)

     
  • John May

    John May - 2013-06-28

    Would it be possible skim through the module shuffling stuff first - 644. Don't need a full review but I'd already made the pdbcml test suite as part of that patch and seems a bit silly if we both patch the same things.

    I do need to break it up (in particular the ioformats I need to think more about) but there some stuff in that's holding me up from fixing other stuff/bugs - ala smiles.

     
  • Egon Willighagen

    • status: open --> closed
    • Group: Needs_Review --> Accepted
     
  • Egon Willighagen

    Closed. Looks good.

     
  • Egon Willighagen

    • status: closed --> open
    • Group: Accepted --> Needs_Review
     
  • Egon Willighagen

    Oops... premature... forgot the second branch...

     
  • John May

    John May - 2013-08-01

    Okay, I'll remove the ignored tests and what would you like to do about the deprecated 'boolean' option for logger?

    J

    On 1 Aug 2013, at 16:29, Egon Willighagen egonw@users.sf.net wrote:

    Second branch looks good too, except for a few minor comments... https://github.com/johnmay/cdk/commits/feature/improved-all-rings-finder

    Gonna apply it to master now.

    [patches:#651] AllCycles/AllRingsFinder

    Status: open
    Labels: rings AllRingsFinder
    Created: Fri Jun 28, 2013 10:54 AM UTC by John May
    Last Updated: Thu Aug 01, 2013 02:29 PM UTC
    Owner: nobody

    Okay split this in to two braches...

    [feature/all-cycles] is the new implementation (working on a 'graph') - I also modified the existing cycles so they all give the same representation. That is they start and end with the same vertex {0, 1, 2, 3, 4, 5, 0} = 6 member ring. A couple of tests had to be changed but it's better to be consistent. first commit - a26c853
    [feature/improved-all-rings-finder] use the new implementation to replace the AllRingsFinder version. I've kept the API the same (except of an Integer to int conversion but that is minimal). I was going to wait and do all the ring sets (i.e. for the other cycle perception) at once but decided this was quite important. The way the conversion is done might changed but that's class private so we can do that it future. first commit - 1e7d94f
    Oh and needs the guava patch.

    Sent from sourceforge.net because cdk-patches@lists.sourceforge.net is subscribed to https://sourceforge.net/p/cdk/patches/

    To unsubscribe from further messages, a project admin can change settings at https://sourceforge.net/p/cdk/admin/patches/options. Or, if this is a mailing list, you can unsubscribe from the mailing list.


    Get your SQL database under version control now!
    Version control is standard for application code, but databases havent
    caught up. So what steps can you take to put your SQL databases under
    version control? Why should you start doing it? Read more to find out.
    http://pubads.g.doubleclick.net/gampad/clk?id=49501711&iu=/4140/ostg.clktrk_______
    Cdk-patches mailing list
    Cdk-patches@lists.sourceforge.net
    https://lists.sourceforge.net/lists/listinfo/cdk-patches

     
  • Egon Willighagen

    Please remove it.

    Egon

    On Thu, Aug 1, 2013 at 5:55 PM, John May jwmay@users.sf.net wrote:

    Okay, I'll remove the ignored tests and what would you like to do about the
    deprecated 'boolean' option for logger?

    J

    On 1 Aug 2013, at 16:29, Egon Willighagen egonw@users.sf.net wrote:

    Second branch looks good too, except for a few minor comments...
    https://github.com/johnmay/cdk/commits/feature/improved-all-rings-finder

    Gonna apply it to master now.

    [patches:#651] AllCycles/AllRingsFinder

    Status: open
    Labels: rings AllRingsFinder
    Created: Fri Jun 28, 2013 10:54 AM UTC by John May
    Last Updated: Thu Aug 01, 2013 02:29 PM UTC
    Owner: nobody

    Okay split this in to two braches...

    [feature/all-cycles] is the new implementation (working on a 'graph') - I
    also modified the existing cycles so they all give the same representation.
    That is they start and end with the same vertex {0, 1, 2, 3, 4, 5, 0} = 6
    member ring. A couple of tests had to be changed but it's better to be
    consistent. first commit - a26c853
    [feature/improved-all-rings-finder] use the new implementation to replace
    the AllRingsFinder version. I've kept the API the same (except of an Integer
    to int conversion but that is minimal). I was going to wait and do all the
    ring sets (i.e. for the other cycle perception) at once but decided this was
    quite important. The way the conversion is done might changed but that's
    class private so we can do that it future. first commit - 1e7d94f
    Oh and needs the guava patch.

    Sent from sourceforge.net because cdk-patches@lists.sourceforge.net is
    subscribed to https://sourceforge.net/p/cdk/patches/

    To unsubscribe from further messages, a project admin can change settings at
    https://sourceforge.net/p/cdk/admin/patches/options. Or, if this is a
    mailing list, you can unsubscribe from the mailing list.


    Get your SQL database under version control now!
    Version control is standard for application code, but databases havent
    caught up. So what steps can you take to put your SQL databases under
    version control? Why should you start doing it? Read more to find out.
    http://pubads.g.doubleclick.net/gampad/clk?id=49501711&iu=/4140/ostg.clktrk_
    Cdk-patches mailing list
    Cdk-patches@lists.sourceforge.net
    https://lists.sourceforge.net/lists/listinfo/cdk-patches


    [patches:#651] AllCycles/AllRingsFinder

    Status: open
    Labels: rings AllRingsFinder
    Created: Fri Jun 28, 2013 10:54 AM UTC by John May
    Last Updated: Thu Aug 01, 2013 03:29 PM UTC
    Owner: nobody

    Okay split this in to two braches...

    [feature/all-cycles] is the new implementation (working on a 'graph') - I
    also modified the existing cycles so they all give the same representation.
    That is they start and end with the same vertex {0, 1, 2, 3, 4, 5, 0} = 6
    member ring. A couple of tests had to be changed but it's better to be
    consistent. first commit - a26c853
    [feature/improved-all-rings-finder] use the new implementation to replace
    the AllRingsFinder version. I've kept the API the same (except of an Integer
    to int conversion but that is minimal). I was going to wait and do all the
    ring sets (i.e. for the other cycle perception) at once but decided this was
    quite important. The way the conversion is done might changed but that's
    class private so we can do that it future. first commit - 1e7d94f

    Oh and needs the guava patch.


    Sent from sourceforge.net because you indicated interest in
    https://sourceforge.net/p/cdk/patches/651/

    To unsubscribe from further messages, please visit
    https://sourceforge.net/auth/subscriptions/

    --
    Dr E.L. Willighagen
    Postdoctoral Researcher
    Department of Bioinformatics - BiGCaT
    Maastricht University (http://www.bigcat.unimaas.nl/)
    Homepage: http://egonw.github.com/
    LinkedIn: http://se.linkedin.com/in/egonw
    Blog: http://chem-bla-ics.blogspot.com/
    PubList: http://www.citeulike.org/user/egonw/tag/papers

     
  • Egon Willighagen

    • status: open --> closed
    • Group: Needs_Review --> Accepted
     
  • Egon Willighagen

    Second part pushed too now.

     

Log in to post a comment.