Skip to content

Speed up io.votable Boolean array conversions - #18455

Merged
bsipocz merged 1 commit into
astropy:mainfrom
eerovaher:faster-votable-converters
Dec 24, 2025
Merged

Speed up io.votable Boolean array conversions#18455
bsipocz merged 1 commit into
astropy:mainfrom
eerovaher:faster-votable-converters

Conversation

@eerovaher

@eerovaher eerovaher commented Jul 23, 2025

Copy link
Copy Markdown
Member

Description

#18454 is proposing speeding up parts of io.votable by rewriting a few Python implementations in Cython. Here I am proposing to replace some of the Python implementations with numpy function calls. For now I am only rewriting two Boolean array conversion functions. I have not looked at the other functions yet, so I am not sure right now if they can be sped up in a similar manner or not, but I decided to open this pull request early so that it can compete with #18454.

Timing setup

$ cat timing_setup.py 
import numpy as np

from astropy.io.votable.converters import bitarray_to_bool, bool_to_bitarray

smaller_arr = np.random.default_rng(486).random(50) > 0
larger_arr = np.random.default_rng(486).random(5000) > 0
smaller_bit_arr = bool_to_bitarray(smaller_arr)
larger_bit_arr = bool_to_bitarray(larger_arr)
$ ipython -i timing_setup.py 
Python 3.12.3 (main, Jun 18 2025, 17:59:45) [GCC 13.3.0]
Type 'copyright', 'credits' or 'license' for more information
IPython 9.2.0 -- An enhanced Interactive Python. Type '?' for help.

Timing results on current main (8599d49)

In [1]: %timeit bool_to_bitarray(smaller_arr)
2.34 μs ± 67.7 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

In [2]: %timeit bool_to_bitarray(larger_arr)
188 μs ± 86.9 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)

In [3]: %timeit bitarray_to_bool(smaller_bit_arr, length=50)
4.14 μs ± 30.7 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

In [4]: %timeit bitarray_to_bool(larger_bit_arr, length=5000)
376 μs ± 689 ns per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

Timing results with this patch

In [1]: %timeit bool_to_bitarray(smaller_arr)
1.16 μs ± 3 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [2]: %timeit bool_to_bitarray(larger_arr)
5.6 μs ± 6.68 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

In [3]: %timeit bitarray_to_bool(smaller_bit_arr, length=50)
1.21 μs ± 1.69 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [4]: %timeit bitarray_to_bool(larger_bit_arr, length=5000)
1.69 μs ± 0.978 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)
  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@eerovaher eerovaher added this to the v7.2.0 milestone Jul 23, 2025
@eerovaher
eerovaher requested review from pllim and tomdonaldson July 23, 2025 16:00
@github-actions

github-actions Bot commented Jul 23, 2025

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@pllim pllim mentioned this pull request Jul 23, 2025
Merged
1 task
@pllim

pllim commented Jul 24, 2025

Copy link
Copy Markdown
Member

astropy/astropy-benchmarks#141 is merged so theoretically next push would trigger a new benchmark run with the new relevant benchmarks. 🤞

@eerovaher eerovaher added the benchmark Run benchmarks for a PR label Jul 24, 2025
@pllim
pllim force-pushed the faster-votable-converters branch from 0511ee3 to 9fd26a4 Compare July 26, 2025 13:42
@pllim

pllim commented Jul 26, 2025

Copy link
Copy Markdown
Member

Hmm this PR does not appear to make any difference wrt to the new benchmarks added, unlike #18454 . Should we close without merge?

@astrofrog

astrofrog commented Jul 28, 2025

Copy link
Copy Markdown
Member

I'd like @eerovaher to have a chance to try and see why this does not affect the benchmark before closing. It would be a lot nicer to rely on Numpy if we can rather than introducing yet more C/Cython code.

@eerovaher

eerovaher commented Jul 28, 2025

Copy link
Copy Markdown
Member Author

I don't know much about neither VOTable nor the implementation details of io.votable. The names of the functions I updated made me think they have something to do with Boolean array conversions, but grepping for their names revealed that they are called from BitArray methods, which is apparently separate from BooleanArray. These functions are also called from io.votable.tree.TableElement, but based on the variable names around these calls they have something to do with masked data.

It is not obvious to me that BitArray and masked data are covered by our benchmarks, so it might be necessary to expand the benchmarks before anything can be decided about this pull request.

@stvoutsin stvoutsin mentioned this pull request Jul 29, 2025
Merged
@eerovaher
eerovaher force-pushed the faster-votable-converters branch from 9fd26a4 to e436c09 Compare October 22, 2025 12:25
@eerovaher

eerovaher commented Oct 22, 2025

Copy link
Copy Markdown
Member Author

I rebased this PR now that there are benchmarks for the functionality being modified, but those benchmarks are so slow that they were aborted, so we still can't compare timings.

@astrofrog

astrofrog commented Oct 22, 2025

Copy link
Copy Markdown
Member

The benchmark has failed for both before and after cases it seems?

@eerovaher

eerovaher commented Oct 24, 2025

Copy link
Copy Markdown
Member Author

Yes indeed, so the problem is not with this pull request but with the benchmarks themselves.

@pllim

pllim commented Oct 28, 2025

Copy link
Copy Markdown
Member

Please see

@astrofrog astrofrog modified the milestones: v7.2.0, v8.0.0 Nov 3, 2025
@eerovaher
Speed up votable Boolean array conversions
5447b3a 
Calling `numpy` functions is faster than bit fiddling in Python.
@eerovaher
eerovaher force-pushed the faster-votable-converters branch from e436c09 to 5447b3a Compare December 19, 2025 13:37
bsipocz
bsipocz approved these changes Dec 24, 2025
View reviewed changes

@bsipocz bsipocz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While these two functions are only a small portion of the improvements in #18454, I nevertheless share the sentiment that more numpy and less Cython and C is better if we can have similar performance with np.
So lets go ahead and merge this now, and then we can rebase #18454 and cleanup if needed.

Thanks @eerovaher!

@bsipocz
bsipocz merged commit 55f89b0 into astropy:main Dec 24, 2025
33 checks passed
@eerovaher
eerovaher deleted the faster-votable-converters branch February 26, 2026 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bsipocz bsipocz bsipocz approved these changes
@pllim pllim Awaiting requested review from pllim
@tomdonaldson tomdonaldson Awaiting requested review from tomdonaldson

Assignees

No one assigned

Labels

benchmark Run benchmarks for a PR io.votable no-changelog-entry-needed Performance

Projects

None yet

Milestone

v8.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@eerovaher @pllim @astrofrog @bsipocz