TopicSegmentationRegularizer implementation in biagartm9.0 #917

Seriont · 2018-07-19T09:44:31Z

No description provided.

update

MelLain · 2018-07-19T11:48:32Z

python/artm/regularizers.py


        if background_topic_names is not None:
            if isinstance(background_topic_names, string_types):
                background_topic_names = [background_topic_names]
            for topic_name in background_topic_names:
                self._config.background_topic_names.append(topic_name)
-        elif config is not None and len(config.background_topic_names):


Why did you delete loading of parameters from config? Also you should add class fields for new parameters, add setters/getters and make parameter loading from config for them too.

MelLain · 2018-07-19T11:49:04Z

src/artm/regularizer/topic_segmentation_ptdw.cc

@@ -1,9 +1,10 @@
 // Copyright 2017, Additive Regularization of Topic Models.

-// Author: Anastasia Bayandina (anast.bayandina@gmail.com)
+// Author: Anastasia Bayandina


Add yourself here with email

MelLain · 2018-07-19T11:50:06Z

src/artm/regularizer/topic_segmentation_ptdw.cc

@@ -16,107 +17,272 @@ namespace regularizer {

 void TopicSegmentationPtdwAgent::Apply(int item_index, int inner_iter,
                                       ::artm::utility::LocalPhiMatrix<float>* ptdw) const {
-  const int local_token_size = ptdw->no_rows();
-  const int topic_size = ptdw->no_columns();
+  int local_token_size = ptdw->no_rows();


Why did you remove const qualifiers?

MelLain · 2018-07-19T11:50:59Z

src/artm/regularizer/topic_segmentation_ptdw.cc

        if (is_background_topic[k]) {
          background_probability[i] += local_ptdw_ptr[k];
        }
      }
    }
  }
+  //for (int i = 0; i < local_token_size; ++i) {


I see, that this code is dirty, but try not to pull request temp comments, you can store them in a local branch (thx git:))

MelLain · 2018-07-19T11:51:23Z

src/artm/regularizer/topic_segmentation_ptdw.cc

+  float tau = tau_;
+  float alpha = config_.merge_threshold();
+  bool merge_into_segments = config_.merge_into_segments();
+  std::list<int> &dot_positions = dot_positions_[item_index];


type& variable;

MelLain · 2018-07-19T11:57:07Z

src/artm/regularizer/topic_segmentation_ptdw.cc

 std::shared_ptr<RegularizePtdwAgent>
 TopicSegmentationPtdw::CreateRegularizePtdwAgent(const Batch& batch,
                                                 const ProcessBatchesArgs& args, float tau) {
-  TopicSegmentationPtdwAgent* agent = new TopicSegmentationPtdwAgent(config_, args, tau);
+  std::vector< std::list<int> > dot_positions;


no need in space into template angles

MelLain · 2018-07-19T11:57:23Z

src/artm/regularizer/topic_segmentation_ptdw.cc

+      int token_id = item.transaction_token_id(token_index);
+      if (batch.token(token_id) == ".") {
+        current_dots.push_back(token_index);
+        dot_count += 1;


MelLain · 2018-07-19T11:57:40Z

src/artm/regularizer/topic_segmentation_ptdw.h

@@ -20,18 +20,21 @@ class TopicSegmentationPtdwAgent : public RegularizePtdwAgent {
  friend class TopicSegmentationPtdw;
  TopicSegmentationPtdwConfig config_;
  ProcessBatchesArgs args_;
+  float tau_;
+  mutable std::vector< std::list<int> > dot_positions_;


remove spaces

MelLain · 2018-07-19T12:00:45Z

src/artm/regularizer/topic_segmentation_ptdw.h

-      : config_(config)
-      , args_(args) { }
+  TopicSegmentationPtdwAgent(const TopicSegmentationPtdwConfig& config, const ProcessBatchesArgs& args,
+    double tau, std::vector< std::list<int> > dot_positions)


return parameters as column
: par1
, par2

and add space between {}

MelLain · 2018-07-19T12:00:58Z

src/artm/regularizer/topic_segmentation_ptdw.h


  virtual void Apply(int item_index, int inner_iter, ::artm::utility::LocalPhiMatrix<float>* ptdw) const;
 };

 class TopicSegmentationPtdw : public RegularizerInterface {
 public:
-  explicit TopicSegmentationPtdw(const TopicSegmentationPtdwConfig& config) : config_(config) { }
+  explicit TopicSegmentationPtdw(const TopicSegmentationPtdwConfig& config)
+  : config_(config) {}


Merge

MelLain

Seems good! There're some minor notes, the most important is to replace it++ to ++it for every iterator in code, as first does unnecessary copy

MelLain · 2018-09-02T06:10:04Z

src/artm/regularizer/topic_segmentation_ptdw.cc


-// Author: Anastasia Bayandina (anast.bayandina@gmail.com)
+// Author: Nikolay Skachkov


email, please)

MelLain · 2018-09-02T06:10:30Z

src/artm/regularizer/topic_segmentation_ptdw.cc


 #include <vector>
 #include <algorithm>
+#include <cmath>


c headers should go first in list of imports

MelLain · 2018-09-02T06:10:43Z

src/artm/regularizer/topic_segmentation_ptdw.cc

@@ -14,109 +15,186 @@
 namespace artm {
 namespace regularizer {

+


no need in this empty line

MelLain · 2018-09-02T06:10:56Z

src/artm/regularizer/topic_segmentation_ptdw.cc

  std::vector<float> background_probability(local_token_size, 0.0f);
-
-  // if background topics are given, count probability for each word to be background
+  //// if background topics are given, count probability for each word to be background


MelLain · 2018-09-02T06:11:40Z

src/artm/regularizer/topic_segmentation_ptdw.cc

+  std::list<int>& dot_positions = dot_positions_[item_index];
+  float average_len = 0.0f;
+  int seg_begin = 0;
+  for (auto it = dot_positions.begin(); it != dot_positions.end(); it++) {


it++ -> ++it

MelLain · 2018-09-02T06:13:14Z

src/artm/regularizer/topic_segmentation_ptdw.cc

+    sen_begin = sen_end + 1;
+    prev_sen_subject = sen_subj;
+    prev_seg_len = sen_len;
+    it++;


it++ -> ++it

MelLain · 2018-09-02T06:13:31Z

src/artm/regularizer/topic_segmentation_ptdw.cc

-        (*ptdw)(i, k) = 0.0f;
+    sigma /= static_cast<int>(distances.size());
+    int i = 0;
+    std::list<int>::iterator it = dot_positions.begin();


MelLain · 2018-09-02T06:13:40Z

src/artm/regularizer/topic_segmentation_ptdw.cc

+      if (distances[i] > mean_dist + alpha * sigma) {
+        it = dot_positions.erase(it);
+        count++;
+        it--;


MelLain · 2018-09-02T06:14:44Z

src/artm/regularizer/topic_segmentation_ptdw.h

+    : config_(config),
+    args_(args),
+    tau_(tau),
+    dot_positions_(dot_positions) { }


: a1
, a2
, a3
...

MelLain · 2018-09-02T06:15:04Z

src/artm/regularizer/topic_segmentation_ptdw.h


  virtual void Apply(int item_index, int inner_iter, ::artm::utility::LocalPhiMatrix<float>* ptdw) const;
 };

 class TopicSegmentationPtdw : public RegularizerInterface {
 public:
-  explicit TopicSegmentationPtdw(const TopicSegmentationPtdwConfig& config) : config_(config) { }
+  explicit TopicSegmentationPtdw(const TopicSegmentationPtdwConfig& config)
+  : config_(config) { }


return into one line

Seriont and others added 3 commits May 11, 2018 01:27

Merge pull request #3 from bigartm/master

69b7c87

update

TopicSegmentationRegularizer new implementation

e98d6ac

segment_reg implemented in bigartm9.0

79f2093

MelLain requested changes Jul 19, 2018

View reviewed changes

nick-skachkov and others added 8 commits July 29, 2018 17:56

TopicSegmentation implemented. Bug fixed

50dcfe6

Merge branch 'master' into master

82ccc98

codestyle fixed

14f204b

Python tests fixed

c152918

Python pep8 fixed

bb614ac

Merge branch 'master' of https://github.com/Seriont/bigartm

86096be

Merge pull request #6 from bigartm/master

331fc5d

Merge

Synchronized with the newest artm version

6d682e5

MelLain reviewed Sep 2, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TopicSegmentationRegularizer implementation in biagartm9.0 #917

TopicSegmentationRegularizer implementation in biagartm9.0 #917

Seriont commented Jul 19, 2018

MelLain Jul 19, 2018

MelLain Jul 19, 2018

MelLain Jul 19, 2018

MelLain Jul 19, 2018

MelLain Jul 19, 2018

MelLain Jul 19, 2018

MelLain Jul 19, 2018

MelLain Jul 19, 2018

MelLain Jul 19, 2018

MelLain Jul 19, 2018

MelLain left a comment

MelLain Sep 2, 2018

MelLain Sep 2, 2018

MelLain Sep 2, 2018

MelLain Sep 2, 2018

MelLain Sep 2, 2018

MelLain Sep 2, 2018

MelLain Sep 2, 2018

MelLain Sep 2, 2018

MelLain Sep 2, 2018

MelLain Sep 2, 2018


		// Author: Anastasia Bayandina (anast.bayandina@gmail.com)
		// Author: Nikolay Skachkov

		@@ -14,109 +15,186 @@
		namespace artm {
		namespace regularizer {

TopicSegmentationRegularizer implementation in biagartm9.0 #917

Are you sure you want to change the base?

TopicSegmentationRegularizer implementation in biagartm9.0 #917

Conversation

Seriont commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MelLain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment