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

TimeSeries implements core::iter::ExactSizeIterator but doesn't provide upper bound on size_hint #131

Closed
d3v-null opened this issue Jul 14, 2022 · 4 comments · Fixed by #134
Assignees

Comments

@d3v-null
Copy link

d3v-null commented Jul 14, 2022

Hello!
Big fan of this library by the way, love your work.

I wanted to see if I could get the length of a TimeSeries without collecting into a Vec, but when I call .len() I get the error below.

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `None`,
 right: `Some(0)`', /rustc/fe5b13d681f25ee6474be29d748c65adcd91f69e/library/core/src/iter/traits/exact_size.rs:108:9
stack backtrace:
   0: rust_begin_unwind
             at /rustc/fe5b13d681f25ee6474be29d748c65adcd91f69e/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/fe5b13d681f25ee6474be29d748c65adcd91f69e/library/core/src/panicking.rs:143:14
   2: core::panicking::assert_failed_inner
   3: core::panicking::assert_failed
             at /rustc/fe5b13d681f25ee6474be29d748c65adcd91f69e/library/core/src/panicking.rs:182:5
   4: core::iter::traits::exact_size::ExactSizeIterator::len
             at /rustc/fe5b13d681f25ee6474be29d748c65adcd91f69e/library/core/src/iter/traits/exact_size.rs:108:9
   5: hifitimetest::main
             at ./src/bin/hifitimetest.rs:9:10
   6: core::ops::function::FnOnce::call_once
             at /rustc/fe5b13d681f25ee6474be29d748c65adcd91f69e/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

This seems to be because ExactSizeIterator is expecting an upper bound on size_hint, which I found to always be None.

If there's no intention of providing .len(), and TimeSeries impls ExactSizeIterator for some other reason, then that's understandable, but it's just not clear from the docco.

Here's some code to demo what I mean.

use hifitime::{Duration, Epoch, TimeSeries, Unit::Microsecond};

fn main() {
    let start = Epoch::from_gregorian_str("2022-07-14T02:56:11.228271007 UTC").unwrap();
    let step = Duration::from_f64(0.5, Microsecond);
    let steps = 100_000_000_000_000;
    let end = start + steps * step;
    let times = TimeSeries::exclusive(start, end, step);
    dbg!(times.size_hint());
    assert_eq!(times.len(), steps as usize);
}

my cargo.toml:

[package]
...
edition = "2021"
rust-version = "1.60"

[dependencies]
hifitime = "3.2.0"

I get

[src/bin/hifitimetest.rs:8] times.size_hint() = (
    0,
    None,
)

and then the error above.

Not a big deal since you can do .count(), it just consumes the iterator and takes a surprisingly long time.

fn main() {
    let start = Epoch::from_gregorian_str("2022-07-14T02:56:11.228271007 UTC").unwrap();
    let step = Duration::from_f64(0.5, Microsecond);
    let steps = 1_000_000_000;
    let end = start + 1_000_000_000 * step;
    let times = TimeSeries::exclusive(start, end, step);

    let timer_start = Epoch::now().unwrap();
    dbg!(times.size_hint());
    assert_eq!(times.count(), steps as usize);
    let timer_end = Epoch::now().unwrap();
    eprintln!("took {}seconds", (timer_end - timer_start).in_seconds());
}
[src/bin/hifitimetest.rs:11] times.size_hint() = (
    0,
    None,
)
took 65.855730688seconds

version info

➜ cargo -Vv
cargo 1.61.0 (a028ae4 2022-04-29)
release: 1.61.0
commit-hash: a028ae42fc1376571de836be702e840ca8e060c2
commit-date: 2022-04-29
host: x86_64-unknown-linux-gnu
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.80.0-DEV (sys:0.4.51+curl-7.80.0 vendored ssl:OpenSSL/1.1.1m)
os: Arch Linux [64-bit]

➜ cargo tree --package hifitime
hifitime v3.2.0
├── libm v0.2.2
├── regex v1.6.0
│   ├── aho-corasick v0.7.18
│   │   └── memchr v2.5.0
│   ├── memchr v2.5.0
│   └── regex-syntax v0.6.27
├── serde v1.0.139
└── serde_derive v1.0.139 (proc-macro)
    ├── proc-macro2 v1.0.40
    │   └── unicode-ident v1.0.1
    ├── quote v1.0.20
    │   └── proc-macro2 v1.0.40 (*)
    └── syn v1.0.98
        ├── proc-macro2 v1.0.40 (*)
        ├── quote v1.0.20 (*)
        └── unicode-ident v1.0.1

Thanks!

@ChristopherRabotin
Copy link
Member

ChristopherRabotin commented Jul 14, 2022 via email

@d3v-null
Copy link
Author

No worries! enjoy your vacation!

@ChristopherRabotin
Copy link
Member

ChristopherRabotin commented Jul 31, 2022

Sorry for the delay here, I'm working on this now. I hope to get it fixed very soon and it's scheduled in next weekend's release: https://github.com/nyx-space/hifitime/milestone/8 .

Let me know if there are other iterator functions you expect to use so I can make sure that those aren't buggy like size_hint is.

@ChristopherRabotin
Copy link
Member

Sorry I only got to this now. I think I've fixed it, and used your test case in the tests themselves (although I changed the number of steps so I could do the math in my head): https://github.com/nyx-space/hifitime/pull/134/files#diff-1ddffea2426780693e1e8ab9f0cf7dcf9361f71126303ebef15d39d919e03cedR173 . I think this is correct but let me know if it isn't.

Thanks again for the bug report!

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 a pull request may close this issue.

2 participants