search

astropy / astropy-benchmarks

Benchmarks for the astropy project
https://spacetelescope.github.io/bench/astropy-benchmarks/
BSD 3-Clause "New" or "Revised" License
7 stars 27 forks source link

Fix mem profiling error for ASCII Table #91

Closed pllim closed 4 years ago

pllim commented 4 years ago

Fix #87

Many thanks to @pv for the help!

Bonus: PEP 8

cc @mhvk (somehow unable to add you as reviewer)

mhvk commented 4 years ago

Nice sleuthing to understand the problem, but I do worry this now tests something else than intended... So, better to have whoever designed the benchmark have a look...

pllim commented 4 years ago

Git blames it to @mdmueller , who added the benchmarks 5 years ago. 🤷‍♀

pllim commented 4 years ago

I don't understand the test error:

asv: error: unrecognized arguments: --strict
astrofrog commented 4 years ago

I think it must be that asv is too old on some configs

bsipocz commented 4 years ago

Unrelated to this PR, but I noticed here that the travis testing runs forever. Should we indeed run such thorough testing, or maybe just do one or two of the import benchmarks would do the trick?

pllim commented 4 years ago

I don't see any error in the log but it still failed. Any more ideas?

astrofrog commented 4 years ago

As @mhvk said, it could be that with these changes we aren't testing the original intent of this test - since @mdmueller may not be able to review this, I'll tag @taldcroft for input.

astrofrog commented 4 years ago

@bsipocz

Unrelated to this PR, but I noticed here that the travis testing runs forever. Should we indeed run such thorough testing, or maybe just do one or two of the import benchmarks would do the trick?

The builds take 3-6 minutes each which doesn't seem too long especially since this repo is pretty low activity. I think it's important we test all the benchmarks since otherwise they will silently fail on the benchmark machine. Note that we run the benchmarks in quick mode here, which means we only run them once each, not multiple times to get an average as they do on the real machine.

bsipocz commented 4 years ago

The builds take 3-6 minutes each

it's more like 20+mins each, thus I was asking

astrofrog commented 4 years ago

Yeah something is wrong... it was 3-6 a week ago

taldcroft commented 4 years ago

If the mem_table_init is just about finding the memory size of the table, then doing that as_array should be fine.

pllim commented 4 years ago

I reverted my change to .travis.yml as fixing it is out of scope. Thanks for the review, @taldcroft !