add from_underlying and get_underlying methods#38
add from_underlying and get_underlying methods#38niklebedenko wants to merge 1 commit intodavehadley:developfrom
from_underlying and get_underlying methods#38Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #38 +/- ##
===========================================
- Coverage 91.82% 90.45% -1.37%
===========================================
Files 20 20
Lines 1125 1079 -46
===========================================
- Hits 1033 976 -57
- Misses 92 103 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
davehadley
left a comment
davehadley
left a comment
There was a problem hiding this comment.
Thanks for your contribution, @niklebedenko.
There are many missing methods that it would be useful to add to these structs. In principle I have no objection to adding these. I attached some inline comments with the code.
I don't like the nomenclature of *_underlying. Looking around for an analogy in the Rust standard library we have String::from_utf8 and String::as_bytes.
So perhaps, from_vec / from_map and as_vec / as_map would be better names?
|
|
||
| /// Constructs a new histogram from the underlying data | ||
| pub fn from_underlying(axes: A, values: HashMap<usize, V>) -> Self { | ||
| Self { axes, values } |
There was a problem hiding this comment.
How should this behave if values contains an entry for usize that does not correspond to a valid bin index in the provided axes?
|
|
||
| /// Returns `None` if the number of values does not match the | ||
| /// expected number of bins for the given axes. | ||
| pub fn from_underlying(axes: A, values: Vec<V>) -> Option<Self> { |
There was a problem hiding this comment.
I think that it would be better if this returned a Result rather than an Option. There is already a AxisError::InvalidNumberOfBins that might be an appropriate thing to return here.
|
@niklebedenko is this pull request still in active development? |
2 participants
Hi there, I'm loving this crate -- there was just one little thing that I really needed: a constructor & getter for the underlying data. I needed this so that I could implement
rkyvtraits for your types. The alternative would be to addrkyvas a dependency to your crate (behind a feature flag) and add its#[derive]macros everywhere. Instead, I thought that adding a couple extra methods would hopefully mean that nobody else needs to add their own favourite macros.