Saving and loading data from the SQL database rely on _adapt_xxx and _convert_xxx methods. However, currently they are using many numpy functions (np.isnan, np.isinf, np.save, np.load) that are slow when dealing with arrays containing single values. This PR makes the following changes that significantly improve data loading and saving performance:

For float: Replace numpy functions by math functions, use math.isinfinite instead of combining isnan and isinf;
For complex: Do not save complex single values as numpy arrays of one value. It saves both time and space if we avoid reading and writing the relatively large numpy header. This changes the representation of the data in the database, but it's fully backwards compatible (and tested);
For array: Use directly functions from np.lib.format, it skips some work of np.save and np.load;

b0b6528 rewrites adapters and converters avoiding numpy functions and significantly improves performance (around x3 when loading regular numeric data and up to x70 for complex data).

20c58d7 is a minor improvement (~10% when loading complex or numeric data), it avoids unpacking sqlite3.Row to list when it's unnecessary (those functions can reorder the columns however, it's most of the time unnecessary because the SQL queries are properly sorted).

3d7b0fd follows the previous one: as the SQL queries are properly sorted, the named tuple feature of sqlite3.Row (row[column_name]) becomes superfluous.

459b342 is up-to-date benchmarks. (Note that the benchmarks are not run in CI.)

Commit b0b6528 is the most important one, the two other modifications give pretty minor improvements compared to the number of changes to the codebase. If you feel that it's not worth changing so much for such a small gain, they can be left out.

(we worked on these preformance improvements together with @mgunyho)

closes #1238 (?)

Aug 01 '22 14:08 fblanchetNaN

Codecov Report

Merging #4446 (3362c91) into master (34f942b) will decrease coverage by 0.02%. The diff coverage is 93.93%.

:exclamation: Current head 3362c91 differs from pull request most recent head b476b79. Consider uploading reports for the commit b476b79 to get more accurate results

@@            Coverage Diff             @@
##           master    #4446      +/-   ##
==========================================
- Coverage   68.23%   68.21%   -0.03%     
==========================================
  Files         339      339              
  Lines       31801    31778      -23     
==========================================
- Hits        21700    21676      -24     
- Misses      10101    10102       +1

Aug 02 '22 09:08 codecov[bot]

Thanks @fblanchetNaN and @mgunyho I will need a bit of time to look at this so please bear with me

Aug 02 '22 09:08 jenshnielsen

@fblanchetNaN and @mgunyho thanks for the contribution. @astafan8 And I finally had a chance to look at it today. We agreed that this looks good and we would like to merge most of it. We are not completely confident in the logic for scalar complex numbers so we would prefer if we can merge the other optimizations but leave that one for a subsequent pr.

Sep 13 '22 12:09 jenshnielsen

Thanks for the response!

We agree that the complex scalar logic looks a bit convoluted, even though it is just following the numpy format standard. We can take it out from this PR and discuss it further separately, although for us this is actually the more significant change because it really affects the loading times in some of our experiments.

I can remove the changes related to complex stuff and force-push if that sounds good. I guess we anyway need to rebase to fix the conflicts.

Sep 14 '22 08:09 fblanchetNaN

The branch is now updated excluding the changes related to complex numbers

Sep 14 '22 09:09 fblanchetNaN

We just realized that some stuff from the complex adapter/converter was left over (like pointed out here, and also here). Is it ok to rebase and force-push? Or should I add new commits to preserve your review? Sorry about the mess.

Sep 14 '22 10:09 fblanchetNaN

Is it ok to rebase and force-push? Or should I add new commits to preserve your review? Sorry about the mess.

no worries :) both are fine, feel free to do what's most convenient for you, the comments will be preserved anyways.

Sep 14 '22 11:09 astafan8

The branch is now updated including the required changes, all checks have passed.

I have marked the conversations as resolved after replying/accepting them.

Sep 20 '22 11:09 fblanchetNaN

Build succeeded:

Sep 20 '22 12:09 bors[bot]

Significant performance improvements to AddResult / GetParameter

Codecov Report