Remove free_long and free_double #671

Merged
merged 6 commits into from Apr 7, 2014

Conversation

Projects
None yet
4 participants
@unnonouno
Owner

unnonouno commented Feb 17, 2014

I removed free_long and free_double from weighted_point.
Model file compatibility is broken.

+
+template <typename T>
+void sort_by(const std::vector<double>& scores, std::vector<T>& array) {
+ assert(scores.size() == array.size());

This comment has been minimized.

Show comment Hide comment
@suma

suma Feb 24, 2014

Owner

Use jubatus/core/common/assert.hpp.

@suma

suma Feb 24, 2014

Owner

Use jubatus/core/common/assert.hpp.

@@ -50,6 +64,8 @@ void bicriteria_as_coreset(
wplist bic,
const size_t dstsize,
wplist& dst) {
+ assert(dstsize >= dst.size());

This comment has been minimized.

Show comment Hide comment
@suma

suma Feb 24, 2014

Owner

assertion.

@suma

suma Feb 24, 2014

Owner

assertion.

+namespace {
+
+template <typename T, typename S>
+struct sort_by_first {

This comment has been minimized.

Show comment Hide comment
@gintenlabo

gintenlabo Feb 24, 2014

Contributor

sort_by_first appears inappropriate. How about compare_by_first?

@gintenlabo

gintenlabo Feb 24, 2014

Contributor

sort_by_first appears inappropriate. How about compare_by_first?

+ pairs[i].first = scores[i];
+ swap(pairs[i].second, array[i]);
+ }
+ std::sort(pairs.begin(), pairs.end(), sort_by_first<double, T>());

This comment has been minimized.

Show comment Hide comment
@gintenlabo

gintenlabo Feb 24, 2014

Contributor

This template argument appears redundant.

How about modifying sort_by_first's definition like this:

struct sort_by_first {
  template<class T, class S>
  bool operator() const (
      const std::pair<T, S>& p1,
      const std::pair<T, S>& p2) const {
    return p1.first < p2.first;
  }
};

and use sort_by_first()?

@gintenlabo

gintenlabo Feb 24, 2014

Contributor

This template argument appears redundant.

How about modifying sort_by_first's definition like this:

struct sort_by_first {
  template<class T, class S>
  bool operator() const (
      const std::pair<T, S>& p1,
      const std::pair<T, S>& p2) const {
    return p1.first < p2.first;
  }
};

and use sort_by_first()?

@kmaehashi

This comment has been minimized.

Show comment Hide comment
@kmaehashi

kmaehashi Feb 24, 2014

Owner

In the meeting on 2014-02-24, reviewer was not decided.
I've temporarily set the milestone to Near Future as a reminder.

Owner

kmaehashi commented Feb 24, 2014

In the meeting on 2014-02-24, reviewer was not decided.
I've temporarily set the milestone to Near Future as a reminder.

@kmaehashi kmaehashi added this to the Near Future milestone Feb 24, 2014

@kmaehashi kmaehashi added core labels Feb 24, 2014

@kmaehashi

This comment has been minimized.

Show comment Hide comment
@kmaehashi

kmaehashi Feb 24, 2014

Owner

Discussion from meeting on 2014-02-24:

  • This pull-req is a improvement to eliminate variables used during calculation.
  • We should note that these variables are currently serialized to the model file. We can pad with dummy variable to keep compatibility.
Owner

kmaehashi commented Feb 24, 2014

Discussion from meeting on 2014-02-24:

  • This pull-req is a improvement to eliminate variables used during calculation.
  • We should note that these variables are currently serialized to the model file. We can pad with dummy variable to keep compatibility.

@kmaehashi kmaehashi added the _updated label Feb 25, 2014

@unnonouno unnonouno changed the title from (WIP) Remove free_long and free_double to Remove free_long and free_double Mar 24, 2014

@unnonouno

This comment has been minimized.

Show comment Hide comment
@unnonouno

unnonouno Mar 24, 2014

Owner

related to #662

Owner

unnonouno commented Mar 24, 2014

related to #662

@kmaehashi kmaehashi self-assigned this Apr 7, 2014

@kmaehashi kmaehashi modified the milestones: 0.5.4, Near Future Apr 7, 2014

kmaehashi added a commit that referenced this pull request Apr 7, 2014

Merge pull request #671 from jubatus/remove_free
Remove free_long and free_double

@kmaehashi kmaehashi merged commit f1fdcf0 into develop Apr 7, 2014

1 check passed

default The Travis CI build passed
Details

@kmaehashi kmaehashi deleted the remove_free branch Apr 7, 2014

@kmaehashi

This comment has been minimized.

Show comment Hide comment
@kmaehashi

kmaehashi Apr 7, 2014

Owner

👍 I confirmed that tests run successfully.

Owner

kmaehashi commented Apr 7, 2014

👍 I confirmed that tests run successfully.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment