Opened 13 years ago

Closed 13 years ago

#292 closed enhancement (fixed)

Interface of PearsonCorrelation does not support iterators

Reported by: Peter Owned by: Peter
Priority: major Milestone: yat 0.4
Component: statistics Version: trunk
Keywords: Cc:

Description (last modified by Markus Ringnér)

Current interface:

double 	score (const classifier::Target &target, const utility::vector &value)
double 	score (const classifier::Target &target, const classifier::DataLookupWeighted1D &value)
double 	score (const classifier::Target &target, const utility::vector &value, const utility::vector &weight)

We have decided:

  • Interfaces should support iterators, whereas old style interfaces with numerous overloaded functions for different containers should be phased out.
  • In the case of Target, functions returning a deque<bool> for binary representation and vector<int> for multiple classes should be provided so that users can use std:iterators for these returned objects, rather than providing an iterator for Target itself.

Change History (7)

comment:1 Changed 13 years ago by Jari Häkkinen

Milestone: To Be Determined0.4

comment:2 Changed 13 years ago by Markus Ringnér

Description: modified (diff)

comment:3 Changed 13 years ago by Peter

This is not only for PearsonCorrelation? but for a whole bunch of classes. Therefore implement this is a template. There is already a template (see ticket:266) in utility.h that doesn't use iterators. Simply replace that template.

It would be good if this worked both for unweighted and weighted iterators.

comment:4 Changed 13 years ago by Peter

Status: newassigned

comment:5 Changed 13 years ago by Peter

I doubt it is a good idea to return a deque<bool> from Target for binary.

The reason is the internal implementation of Target. There are four things:

// label of class i
std::vector<std::string> labels_; 
// binary of class i
std::vector<char> binary_; 
// class of sample i
std::vector<size_t> classes_;
// map from label to class
std::map<std::string,size_t> class_map_;  

One would like to return the binary as a const& and therefore it is tempting to replace binary_ binary of class i with binary of sample i. However doing that causes problem in random_shuffle because currently it just shuffles classes_ as it is only variable that is related to samples (other variable map labels, classes and binary).

It is of course doable to solve this problem, but I don't think it is worth it. What do you think?

Yet we should of course replace passing containers with passing iterators.

comment:6 Changed 13 years ago by Peter

Iteartors added in PearsonCorrelation? in [1139]

comment:7 Changed 13 years ago by Peter

Resolution: fixed
Status: assignedclosed

fixed in [1145]

Placed the function in statistics/utility.

Note: See TracTickets for help on using tickets.