PUBDEV-7736 use btree for coxph computation instead the direct approach #4895

satai · 2020-09-04T14:00:58Z

No description provided.

michalkurka · 2020-09-08T15:53:29Z

h2o-core/src/main/java/hex/ModelMetricsRegressionCoxPH.java

+
+      return byStrata.stream()
+        .map(
+          indexes -> statsForAStrata(startVec, stopVec, eventVec, estimateVec, indexes)


Why is this using the Vec API at all? If you know you have only a very limited number of indices - can it be collected to non-distributed structures and avoid all overhead with using the Vec API to access a single value?

michalkurka · 2020-09-08T15:53:55Z

h2o-core/src/main/java/hex/ModelMetricsRegressionCoxPH.java

@@ -166,64 +174,343 @@ static Stats concordance(final Vec startVec, final Vec stopVec, final Vec eventV
    private static Stats concordanceStats(Vec.Reader startVec, Vec.Reader stopVec, Vec.Reader eventVec, List<Vec.Reader> strataVecs, Vec.Reader estimateVec, long length) {
      assert 0 <= length && length <= Integer.MAX_VALUE;

-      Stream<Integer> allIndexes = IntStream.range(0, (int) length).boxed();
+      Collection<List<Integer>> byStrata =


this should be rewritten to MRTask

As you wish

michalkurka · 2020-09-22T20:10:29Z

h2o-core/src/main/java/hex/ModelMetricsRegressionCoxPH.java

+    }
+
+    private static Frame prepareFrameForConcordanceComputation(Vec eventVec, List<Vec> strataVecs, Vec estimateVec, Vec durations) {
+      final Frame fr = new Frame(Key.make());


does it need to have a key?

michalkurka · 2020-09-22T20:10:42Z

h2o-core/src/main/java/hex/ModelMetricsRegressionCoxPH.java

+      for (int i = 0; i < strataVecs.size(); i++) {
+        fr.add("strata_" + i, strataVecs.get(i));
+      }
+      DKV.put(fr);


probably no need to install into DKV

michalkurka · 2020-09-22T20:11:31Z

h2o-core/src/main/java/hex/ModelMetricsRegressionCoxPH.java

+              .outputFrame(Key.make("durations_" + fr1._key), new String[]{"durations"}, null));
+      Scope.track(frame);
+
+      fr1.delete(false);


michalkurka · 2020-09-22T20:14:48Z

I think there is a lot of unnecessary DKV#puts, you really only need to put a Frame in DKV when it is a user-facing result. Most (if not all) your frames have a temporary nature - for regular MRTasks they don't need to have a key, don't need to be in DKV.

Working with Scope and explicit deletes is suspicious and unexpected.

satai · 2020-09-23T08:09:05Z

@michalkurka you were right, that DKV and Scope ops were not necessary. Pushed a cleanup commit

michalkurka · 2020-10-13T13:39:19Z

h2o-core/src/main/java/hex/ModelMetricsRegressionCoxPH.java

+    }
+
+    private static Vec durations(Vec startVec, Vec stopVec) {
+      Vec vec = null == startVec ? stopVec.makeZero() : startVec;


I don't see the temporary frame disposed of anywhere.

On another note - if startVec is not given then durations = startVec. You can skip the MRTask altogether.

michalkurka · 2020-10-13T13:42:35Z

h2o-core/src/main/java/hex/ModelMetricsRegressionCoxPH.java


-      return stats;
+      withoutNas.replace(1, withoutNas.vec("event").toNumericVec());


removeNAs is also converting event column to numeric?

I would expect such transformations to be done in top-level (public) methods instead of utility functions.

was removed

h2o-docs/src/product/api-changes.rst

michalkurka · 2020-10-13T13:46:47Z

h2o-py/tests/testdir_algos/coxph/pyunit_coxph_concordance.py

@@ -20,8 +20,6 @@ def coxph_smoke():

    metrics = coxph.model_performance(heart)
    assert 0.581 > metrics.concordance() and 0.580 < metrics.concordance()
-    assert 3696 == metrics.concordant()


intentional?

Rebased, the test was redesigned in an other PR to use lifelines instead of hardcoded results.

michalkurka · 2020-10-13T13:47:36Z

h2o-r/tests/testdir_algos/coxph/runit_coxph_concordance.R

@@ -24,7 +24,7 @@ test.CoxPH.concordance <- function() {
    rConcordance <- unname(summary(rModel)$concordance)[1]
    hexConcordance <- h2o.performance(hexModel, data=tstdataHex)@metrics$concordance

-    expect_equal(rConcordance, hexConcordance)
+    expect_equal(rConcordance, hexConcordance, tolerance = 1e-3, scale = 1)


Isn't the tolerance quite high? This is not complex math

It was used in all the coxph tests so far I have seen.

…ch (h2oai#4895) * PUBDEV-7736 use btree for coxph computation instead the direct approach (cherry picked from commit 8612f10)

satai requested a review from michalkurka September 4, 2020 14:00

satai force-pushed the ondra/concordance_performance branch from 8c1517a to e91a22b Compare September 8, 2020 11:17

michalkurka reviewed Sep 8, 2020

View reviewed changes

satai force-pushed the ondra/concordance_performance branch 2 times, most recently from 7d5fc81 to df18ee1 Compare September 22, 2020 09:32

satai requested a review from Pscheidl September 22, 2020 09:36

satai force-pushed the ondra/concordance_performance branch from df18ee1 to 489d632 Compare September 22, 2020 09:41

satai marked this pull request as ready for review September 22, 2020 11:52

michalkurka reviewed Sep 22, 2020

View reviewed changes

michalkurka reviewed Oct 13, 2020

View reviewed changes

h2o-docs/src/product/api-changes.rst Outdated Show resolved Hide resolved

michalkurka reviewed Oct 13, 2020

View reviewed changes

satai force-pushed the ondra/concordance_performance branch 4 times, most recently from 838ba28 to 138e6f9 Compare October 15, 2020 12:09

satai added the please review label Oct 20, 2020

satai force-pushed the ondra/concordance_performance branch from 3b49fcc to 68c3a50 Compare October 20, 2020 10:44

michalkurka approved these changes Oct 26, 2020

View reviewed changes

satai force-pushed the ondra/concordance_performance branch 2 times, most recently from dbbe094 to bf55498 Compare October 29, 2020 11:44

satai and others added 2 commits October 29, 2020 13:11

PUBDEV-7736 use btree for coxph computation instead the direct approach

b8ad44d

pUBDEV-7759: Updated API 3.30.1.1 section

c41a75e

satai added 4 commits October 29, 2020 13:11

PUBDEV-7736 remove dead code and unused params

8f8e441

PUBDEV-7736 remove unused DKV and Scope operations

c8ce4e9

PUBDEV-7736 remove unnecessary MR operations

bda8d69

PUBDEV-7736 remove unnecessary DKV call

c4552a4

satai force-pushed the ondra/concordance_performance branch from 5a5267a to c4552a4 Compare October 29, 2020 12:11

PUBDEV-7736 clean unit test

dc309b8

satai force-pushed the ondra/concordance_performance branch 2 times, most recently from a018ee3 to 5a01a46 Compare October 30, 2020 18:52

PUBDEV-7736 clean unit tests

5685e66

satai force-pushed the ondra/concordance_performance branch from 5a01a46 to 5685e66 Compare November 2, 2020 12:53

satai added 16 commits November 2, 2020 13:59

PUBDEV-7736 clean unit tests

583ee83

PUBDEV-7736 debug unit tests

879fa4b

PUBDEV-7736 debug unit tests

8754abe

PUBDEV-7736 debug unit tests

b0ce9bf

PUBDEV-7736 debug unit tests

46dea95

PUBDEV-7736 debug unit tests

1cfdb8e

PUBDEV-7736 clean unit test

896996d

PUBDEV-7736 debug unit tests

b7d6c5f

PUBDEV-7736 debug unit tests

fd1d29d

PUBDEV-7736 debug unit tests

573e76c

PUBDEV-7736 debug unit tests

d86693e

PUBDEV-7736 debug unit tests

09450df

PUBDEV-7736 debug unit tests

3819dba

PUBDEV-7736 debug unit tests

9476700

PUBDEV-7736 debug unit tests

a4d9adf

PUBDEV-7736 debug unit tests

0e7e674

satai merged commit 8612f10 into master Nov 9, 2020

satai deleted the ondra/concordance_performance branch December 17, 2020 11:38

h2o-ops mentioned this pull request May 14, 2023

CoxPH Concordance score performance #7903

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PUBDEV-7736 use btree for coxph computation instead the direct approach #4895

PUBDEV-7736 use btree for coxph computation instead the direct approach #4895

satai commented Sep 4, 2020

michalkurka Sep 8, 2020

michalkurka Sep 8, 2020

satai Sep 22, 2020

michalkurka Sep 22, 2020

michalkurka Sep 22, 2020

michalkurka Sep 22, 2020

michalkurka commented Sep 22, 2020

satai commented Sep 23, 2020

michalkurka Oct 13, 2020

satai Oct 16, 2020

michalkurka Oct 13, 2020

satai Oct 16, 2020

michalkurka Oct 13, 2020

satai Oct 14, 2020

michalkurka Oct 13, 2020

satai Oct 15, 2020


		return stats;
		withoutNas.replace(1, withoutNas.vec("event").toNumericVec());

PUBDEV-7736 use btree for coxph computation instead the direct approach #4895

PUBDEV-7736 use btree for coxph computation instead the direct approach #4895

Conversation

satai commented Sep 4, 2020

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

michalkurka commented Sep 22, 2020

satai commented Sep 23, 2020

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