Target naming #256
Target naming #256
Conversation
src/dataset/iter.rs
Outdated
| let mut records = self.dataset.records.view(); | ||
| let mut targets = self.dataset.targets.as_targets(); | ||
| let feature_names; | ||
| let mut target_names = vec!["class".to_string()]; |
There was a problem hiding this comment.
Just write let mut target_names;
| use rand::{rngs::SmallRng, SeedableRng}; | ||
|
|
||
| #[test] | ||
| fn set_target_name() { |
There was a problem hiding this comment.
Can you also add target name verification to one or two tests with target_iter just to verify that your iterator changes work correctly?
There was a problem hiding this comment.
addressed lines 551-556 now test this.
src/dataset/impl_dataset.rs
Outdated
| pub fn target_names(&self) -> Vec<String> { | ||
| if !self.target_names.is_empty() { | ||
| self.target_names.clone() | ||
| } else { | ||
| (0..self.ntargets()) | ||
| .map(|idx| format!("class-{}", idx)) | ||
| .collect() |
There was a problem hiding this comment.
If there are no names, this method should instead just return empty list. The signature should be fn target_names(&self) -> &[String] to prevent cloning. Apply this change to feature_names as well for consistency.
| pub fn with_target_names<I: Into<String>>(mut self, names: Vec<I>) -> DatasetBase<R, S> { | ||
| let target_names = names.into_iter().map(|x| x.into()).collect(); | ||
|
|
||
| self.target_names = target_names; | ||
|
|
||
| self | ||
| } |
There was a problem hiding this comment.
This method should check the length of the input vector so that it's equal to number of targets. The input should be Vec so that it can be assigned directly. You can also try implementing this change for feature_names, but that might require more work to change.
There was a problem hiding this comment.
the input for the function is already Vec(I) where I implements Into(String). Unless I am mistaken you want something like?
pub fn with_target_names(mut self, names: Vec<String>) -> DatasetBase<R, S> {
if names.len() == self.ntargets() {
self.target_names = names;
} else {
// raise some error to user stating the number of targets is X or default to class_{0..ntargets}?
}
self
}
There was a problem hiding this comment.
Yeah pretty much. I don't mind panicking here, so you can just assert the condition.
|
This will close #248 |
|
Superseded by #373 |
3 participants
An attempt to add target names per #70. Not really sure what to do with lines 83-98 in iter.rs. Any guidance there would be appreciated.
Line 83 was just added to see if it would build correctly.