search

astropy / astropy-healpix

BSD-licensed HEALPix for Astropy - maintained by @astrofrog and @lpsinger
https://astropy-healpix.readthedocs.io
BSD 3-Clause "New" or "Revised" License
53 stars 22 forks source link

Benchmark and improve performance #22

Open astrofrog opened 7 years ago

astrofrog commented 7 years ago

We should do performance benchmarks of the functionality that also exists in healpy/HEALPIX and make sure that we get comparable results (and improve performance if not).

cdeil commented 7 years ago

@astrofrog - How about simply adding a healpix/bench.py file that can be run via python -m healpix.bench that runs some benchmarks with healpix and healpy and prints a report to the console?

I would prefer this over a big asv setup that's complex to understand and run for a given user / machine / dev.

I could make a minimal PR today if you think it's a good addition for now.

Once that is in place, two ideas we could investigate is whether @cython.wraparound(False) or memoryviews give any performance improvements or whether we should expose (via an option) prange:

Of course it might be that the differences in speed are int he underlying different C implementations, in which case fuzzing with the Cython interface won't help (except for prange, that should give a nice speed-up on multi-core machines, no?).

astrofrog commented 7 years ago

I've actually already set up asv locally, and running it is actually not too complex now (I've written some preliminary benchmarks). I was going to push it to a separate healpix-benchmarks repo and have it auto-run with a cron job, but if you would prefer to have simpler benchmarks, then I think they should live in a separate benchmarks/ directory (not in the installed package).

I did do some timing tests with just the C code and indeed the C code is what is 4x slower, even without Cython (which only adds a few % overhead). The main time is actually spent in converting to/from the xy representation.

astrofrog commented 7 years ago

By the way, I don't think having the benchmarks you are mentioning and asv are incompatible. I think it'd be nice to see at a glance how our speed compares to healpy for the current dev version. Asv is more for getting long term graphs. So we can do both - I'm happy to set up asv in a separate repo if you want to add some scripts in benchmarks to do some simple benchmarking here.

cdeil commented 7 years ago

Speed will always depend on machine, C / C++ compiler, Cython, Numpy, maybe even Astropy version. As a user / developer I want to know how fast this package is (compared to healpy) for my machine (or compute cluster), C / C++ compiler, Cython, Numpy, maybe even Astropy that I plan to use for data analysis.

This use case isn't addressed by asv, it's pretty complex to get up and running, and we don't want to get in the business of setting up a large matrix of systems / versions, that's way too much work to maintain (see how asv went over the past years for other packages).

So I think it would be really nice to put the benchmark functions in the package so that everyone can run python -c healpix.bench any time and know what they have. @astrofrog - If you really want to set up / maintain asv, then we should maybe think about whether it's possible to share the test cases for the two goals. Is it possible to write test cases for asv so that no import asv is needed and they can be benchmarked separately?

cdeil commented 7 years ago

it'd be nice to see at a glance how our speed compares to healpy for the current dev version

Just put python -m healpix.bench at the end of the CI build files and the latest number will always be available and there will be a record over the years how it changes.

astrofrog commented 7 years ago

Is it possible to write test cases for asv so that no import asv is needed and they can be benchmarked separately?

The answer is 'kind of' - but maybe let's not worry about that for now. You are right, we can just put some quick benchmarks in here and if you want I can worry later about making them work also with asv.

Just put python -m healpix.bench at the end of the CI build files and the latest number will always be available and there will be a record over the years how it changes.

I wouldn't trust the values in CI too much since performance of the machine can change over short timescales depending on the overall CI load (since it's just a VM). But in any case you are right, a developer can just check out a specific commit and re-run that command and compare results. So this is fine by me.

Do you want to get this in before 0.1 is released?

cdeil commented 7 years ago

Do you want to get this in before 0.1 is released?

Yes, I'd like to have something minimal. I'll start a PR now.

astrofrog commented 7 years ago

Perfect, thanks! Note that for the pix2ang and ang2pix it would be good to check with different values of nside - there used to be a bug that caused performance to get a lot worse with increasing nside.

cdeil commented 7 years ago

I see you've added a test that runs the benchmark:

astropy_healpix/tests/test_bench.py::test_bench PASSED

It's not clear to me if this is useful. Benchmark will grow and become slow.

FYI: I briefly looked at this today https://pypi.python.org/pypi/pytest-benchmark but then decided to just do the benchmark separately from the tests.

When running the benchmark this way, the result doesn't show up: https://travis-ci.org/astropy/astropy-healpix/jobs/281743909#L909 How about removing the test, and instead adding the line python -m astropy_healpix.bench to the CI config files (at least for one "main" config, the one where also coverage is measured)?

astrofrog commented 7 years ago

It's not clear to me if this is useful

It's mainly to make sure that the code runs. Note that we could have a 'fast' mode for checking that it runs properly where it just runs each function once (not N times to get good stats). If the benchmark code is going to get installed for users then we have to test it.

I considered adding the benchmarks to the CI instead like you mention but then we have to be careful how we run the coverage and combine the coverage files, and this doesn't help the runtime issue. So I do think the option is to add a fast mode to the benchmarks that we can use in the tests.

Alternatively we can split out the benchmarks from the package completely - and then I'm less worried about testing it. But I like where it is at the moment, and I think adding a fast mode would be easy.