Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: introduce AsciiChar wrapper over u8 #302

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

ivan-aksamentov
Copy link
Member

@ivan-aksamentov ivan-aksamentov commented Dec 16, 2024

Followup of #301

The AsciiChar wrapper type allows:

  • better debugging, as we can control how AsciiChar is printed, as opposed to the integer-like u8 type - this is mostly a dev convenience thing - for debug-printing sequences, various maps etc., but it boosts development significantly
  • provide valid outputs wherever we need to display or write to file any characters - this is actually needed for correctness

I measured no significant slowdown:

Command:

$ cargo -q build --release --target-dir=/workdir/.build/docker --bin=treetime
$ hyperfine --warmup 1 --show-output '/workdir/.build/docker/release/treetime ancestral --method-anc=parsimony --dense=false --tree=data/mpox/clade-ii/500/tree.nwk --outdir=tmp/smoke-tests/ancestral/marginal/mpox/clade-ii/500 data/mpox/clade-ii/500/aln.fasta.xz'

Before (branch: rust, commit 67ba54a):

  Time (mean ± σ):     655.4 ms ±  25.7 ms    [User: 2258.9 ms, System: 210.1 ms]
  Range (min … max):   635.3 ms … 717.6 ms    10 runs

After:

  Time (mean ± σ):     650.3 ms ±  26.9 ms    [User: 2231.2 ms, System: 218.7 ms]
  Range (min … max):   630.9 ms … 716.2 ms    10 runs

Followup of #301

The `AsciiChar` wrapper type allows:
 * better debugging, as we can control how `AsciiChar` is printed, as opposed to the integer-like `u8` type - this is mostly a dev convenience thing - for debug-printing sequences, various maps etc., but it boosts development significantly
 * provide valid outputs wherever we need to display or write to file any characters - this is actually needed for correctness

I measured no significant slowdown:

Command:
```
$ cargo -q build --release --target-dir=/workdir/.build/docker --bin=treetime
$ hyperfine --warmup 1 --show-output '/workdir/.build/docker/release/treetime ancestral --method-anc=parsimony --dense=false --tree=data/mpox/clade-ii/500/tree.nwk --outdir=tmp/smoke-tests/ancestral/marginal/mpox/clade-ii/500 data/mpox/clade-ii/500/aln.fasta.xz'
```

Before (branch: rust, commit 67ba54a):
```
  Time (mean ± σ):     655.4 ms ±  25.7 ms    [User: 2258.9 ms, System: 210.1 ms]
  Range (min … max):   635.3 ms … 717.6 ms    10 runs
```

After:
```
  Time (mean ± σ):     650.3 ms ±  26.9 ms    [User: 2231.2 ms, System: 218.7 ms]
  Range (min … max):   630.9 ms … 716.2 ms    10 runs
``
@ivan-aksamentov ivan-aksamentov merged commit 92cd3f8 into rust Dec 16, 2024
14 checks passed
@ivan-aksamentov ivan-aksamentov deleted the refactor/ascii-char branch December 16, 2024 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant