New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve performance of element handling #1601

Merged
merged 35 commits into from Aug 4, 2017

Conversation

Projects
None yet
3 participants
@baoilleach
Member

baoilleach commented Jul 13, 2017

As described over at openbabel/enhancement-proposals#4.

Briefly, the old element handling code had some performance problems. I have written a faster replacement. It has the following additional benefits:

  1. No need for a global etab or isotab and associated initialization.
  2. The elements are now available as consts (e.g. OBElements::Carbon) to replace functions such as IsCarbon().
  3. No need for text/header files in data dir.

Timing comparison: get the symbol for elements 1 to 100 and convert back to atomic number (repeat x10000). 0.82s old vs 0.02s new. A more realistic conversion is to just do carbon: 0.14s old vs 0.01s new.

void test_OBElements()
{
  clock_t start = clock();
  unsigned int tot = 0;
  for (unsigned int N = 0; N < 10000; ++N) {
    for (unsigned int i = 1; i < 100; ++i) {
      const char *elem = OBElements::GetSymbol(6);
      unsigned int atomic_num = OBElements::GetAtomicNum(elem);
      tot += atomic_num;
    }
  }
  clock_t stop = clock();
  double sum = (double)(stop - start) / CLOCKS_PER_SEC;
  printf("%.2f s\n", sum);
  
  OBElementTable etab;
  start = clock();
  tot = 0;
  for (unsigned int N = 0; N < 10000; ++N) {
    for (unsigned int i = 1; i < 100; ++i) {
      const char *elem = etab.GetSymbol(6);
      unsigned int atomic_num = etab.GetAtomicNum(elem);
      tot += atomic_num;
    }
  }
  stop = clock();
  sum = (double)(stop - start) / CLOCKS_PER_SEC;
  printf("%.2f s\n", sum);
}

baoilleach added some commits Jun 17, 2017

@baoilleach

This comment has been minimized.

Show comment
Hide comment
@baoilleach

baoilleach Jul 24, 2017

Member

Updated timings, for the record, with 3 signif figures and 10x more iterations:

Timing comparison: get the symbol for elements 1 to 100 and convert back to atomic number (repeat x100000). 8.05s old vs 0.187s new. A more realistic conversion is to just do carbon: 1.34s old vs 0.078s new.

Member

baoilleach commented Jul 24, 2017

Updated timings, for the record, with 3 signif figures and 10x more iterations:

Timing comparison: get the symbol for elements 1 to 100 and convert back to atomic number (repeat x100000). 8.05s old vs 0.187s new. A more realistic conversion is to just do carbon: 1.34s old vs 0.078s new.

@baoilleach

This comment has been minimized.

Show comment
Hide comment
@baoilleach

baoilleach Aug 4, 2017

Member

Good to merge? Or not...?

Member

baoilleach commented Aug 4, 2017

Good to merge? Or not...?

@ghutchis ghutchis merged commit 4931b9a into openbabel:master Aug 4, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ghutchis

This comment has been minimized.

Show comment
Hide comment
@ghutchis

ghutchis Aug 4, 2017

Member

Yep, it looks good. Sorry for the delay

Member

ghutchis commented Aug 4, 2017

Yep, it looks good. Sorry for the delay

@baoilleach baoilleach deleted the baoilleach:elements branch Sep 4, 2017

@dkoes

This comment has been minimized.

Show comment
Hide comment
@dkoes

dkoes Jan 11, 2018

Contributor

Whhhhhyyyyyyy???

Contributor

dkoes commented on 5e5755e Jan 11, 2018

Whhhhhyyyyyyy???

This comment has been minimized.

Show comment
Hide comment
@baoilleach

baoilleach Jan 11, 2018

Member

Which one?

Member

baoilleach replied Jan 11, 2018

Which one?

This comment has been minimized.

Show comment
Hide comment
@dkoes

dkoes Jan 11, 2018

Contributor

All the simple helper functions (IsSulfur, IsHydrogen, etc) which are easier to read and less error-prone than checking atomic numbers. Of course, I already changed my code to be compatible, but it would have been nice to not have to. This hurts backwards compatibility and I don't see the advantage.

Contributor

dkoes replied Jan 11, 2018

All the simple helper functions (IsSulfur, IsHydrogen, etc) which are easier to read and less error-prone than checking atomic numbers. Of course, I already changed my code to be compatible, but it would have been nice to not have to. This hurts backwards compatibility and I don't see the advantage.

This comment has been minimized.

Show comment
Hide comment
@dkoes

dkoes Jan 11, 2018

Contributor

Also, I'm slightly concerned that IsSingle in OBBond had different semantics than GetBondOrder() == 1...

Contributor

dkoes replied Jan 11, 2018

Also, I'm slightly concerned that IsSingle in OBBond had different semantics than GetBondOrder() == 1...

This comment has been minimized.

Show comment
Hide comment
@baoilleach

baoilleach Jan 11, 2018

Member

Might be best to discuss on the mailing list...

Member

baoilleach replied Jan 11, 2018

Might be best to discuss on the mailing list...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment