Make Atoms() faster via caches

[pmrv]

Fixes #48.

Looking at benchmarks I found that almost all time creating a new Atoms object is spent on either i) reading the periodic table from a csv file and ii) loading mendeleev objects from element strings.

I think it's reasonable to assume that during the lifetime of a pyiron process neither the periodic table changes on disk nor is the mendeleev database updated and therefore cache both on the module level (i.e. even multiple instances of Atoms and PeriodicTable share one cache). In my benchmark that gives a factor 100 speed up.

Caching convert_element has some technical nits with it:

1. I want one cache for all Atoms objects, since
   • each structure might be small, so a per-instance cache wouldn't help a lot (in fact Atoms has a self-rolled cache like that already; I think we can remove it now, but I want to double check with you)
   • we can expect the results from mendeleev to be the same for all instances anyway
2. The expensive part is actually only this, so an alternative would be to make Atom some kind of singleton.
3. At the end of the method it also updates Atoms.species, in the cached case this would not be called. However all call sites of convert_element also call set_species later in the code path, so nothing is lost. It's still messy to rely on this, but I feel this is a separate refactor.

pyiron_atomistics/pyiron_atomistics/atomistics/structure/atoms.py

Line 661 in e69d6df

def convert_element(self, el, pse=None):

[pmrv]

Caching the method on Atoms didn't quite work, so I did the much less intrusive change to cache the call to mendeleev directly. This leaves a factor 2 on the table, but all the nits from above are handled by this.

[sudarsan-surendralal]

This looks good! I made a small commit to fix the failing tests for the Atom class.

Can you also add the benchmarking you have locally as part of the unittests for the Atoms class?

Edit: I had to make more than one commit to fix this ;)

[pmrv]

Thanks for the fix! I will look into how to best do a benchmark as a test case in a system-independent way, probably it's sufficient to time two runs and assert the second time is much faster.

I've attached the profile for this problem at the end here.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[pmrv]

@sudarsan-surendralal Can you have another look? I can't reproduce the test failure outside of the CI.

[coveralls]

Pull Request Test Coverage Report for Build 1056610704

• 10 of 10 (100.0%) changed or added relevant lines in 1 file are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+0.01%) to 68.077%

---

Files with Coverage Reduction	New Missed Lines	%
pyiron_atomistics/atomistics/structure/periodic_table.py	1	83.5%

Totals
Change from base Build 1056270290:	0.01%
Covered Lines:	10908
Relevant Lines:	16023

---

💛 - Coveralls

[sudarsan-surendralal]

@pmrv The tests should work now. I also took the liberty to extend your benchmarks a bit. We seem to consistently get a 15x speedup which is awesome. But I can't seem to get a 100x speedup you found in your benchmarks.

[pmrv]

@pmrv The tests should work now. I also took the liberty to extend your benchmarks a bit. We seem to consistently get a 15x speedup which is awesome. But I can't seem to get a 100x speedup you found in your benchmarks.

I think my benchmark last time was a more elaborate example creating the atoms via get_structure, so maybe there were extra factors that made it even slower. I'll look for it again to confirm the speed up, but 15x looks already very good.

[pmrv]

This is what broke the tests before?

[sudarsan-surendralal]

Yes but not only this. The recent commit b278013 also helped fix the tests

[pmrv]

The tests were failing on mac, so I decreased the expected speedup to 10. Otherwise this looks pretty good! I'll still want to understand what is different now to the original changes, but after that I think we can merge this.

[niklassiemer]

Is there something wrong in the way the speedup is computed (on mac)? In both last cases the threshold was only slightly missed...

[pmrv]

Is there something wrong in the way the speedup is computed (on mac)? In both last cases the threshold was only slightly missed...

Weird, I changed it to 10, because it ~14 before… :S

[niklassiemer]

Is there something wrong in the way the speedup is computed (on mac)? In both last cases the threshold was only slightly missed...

Weird, I changed it to 10, because it ~14 before… :S

That is what I meant with both last cases. And in both cases it was only slightly below. Therefore, I asked if there is an error on computing the time difference on Mac 😆

[pmrv]

Is there something wrong in the way the speedup is computed (on mac)? In both last cases the threshold was only slightly missed...

Weird, I changed it to 10, because it ~14 before… :S

That is what I meant with both last cases. And in both cases it was only slightly below. Therefore, I asked if there is an error on computing the time difference on Mac laughing

Yep, I see that now. I'll try to figure it out.

[pmrv]

Suddenly the speed up test worked for a factor x10 on macOS. I still don't know what's going on, but I've made it hopefully more robust by averaging the times and then checking instead of checking in every iteration.

[pmrv]

On my local machine speed ups are in the range x30-40 now, hope that works on CI as well.

[pmrv]

The merge commit earlier messed up the tests, so I removed it again via a rebase.

[sudarsan-surendralal]

Taking the average of the speedup is definitely a better idea. LGTM!