Fix distributed dbscan with empty partial results #657

Pahandrovich · 2020-06-17T22:21:01Z

No description provided.

Pahandrovich · 2020-06-18T10:05:03Z

/intelci: run

vepifanov · 2020-06-23T14:03:20Z

cpp/daal/src/algorithms/dbscan/dbscan_distributed_input_types.cpp

@@ -129,21 +129,23 @@ services::Status DistributedInput<step2Local>::check(const daal::algorithms::Par
    const size_t nBlocks = dcPartialData->size();
    DAAL_CHECK_EX(nBlocks > 0, ErrorIncorrectNumberOfInputNumericTables, ArgumentName, partialDataStr());


I would suggest to handle case, when nBlocks == 0 in the same manner as the empty partialData tables.

vepifanov · 2020-06-23T14:04:37Z

cpp/daal/src/algorithms/dbscan/dbscan_distributed_input_types.cpp

@@ -129,21 +129,23 @@ services::Status DistributedInput<step2Local>::check(const daal::algorithms::Par
    const size_t nBlocks = dcPartialData->size();
    DAAL_CHECK_EX(nBlocks > 0, ErrorIncorrectNumberOfInputNumericTables, ArgumentName, partialDataStr());

-    size_t nFeatures = 0;
-    for (size_t i = 0; i < nBlocks; i++)
+    if (NumericTable::cast((*dcPartialData)[0])->getNumberOfRows() == 0)


First, this check is incorrect. It should be getNumberOfRows() != 0 to make sense.
Second, it is reasonable to put this check inside loop and just skip tables with 0 rows.

vepifanov · 2020-06-23T14:05:19Z

cpp/daal/src/algorithms/dbscan/dbscan_distributed_input_types.cpp

@@ -249,19 +251,17 @@ services::Status DistributedInput<step3Local>::check(const daal::algorithms::Par
    const size_t nDataBlocks = dcPartialData->size();
    DAAL_CHECK_EX(nDataBlocks > 0, ErrorIncorrectNumberOfInputNumericTables, ArgumentName, partialDataStr());


I would suggest to handle case, when nBlocks == 0 in the same manner as the empty partialData tables.

vepifanov · 2020-06-23T14:05:36Z

cpp/daal/src/algorithms/dbscan/dbscan_distributed_input_types.cpp

-    size_t nFeatures = 0;
-    for (size_t i = 0; i < nDataBlocks; i++)
+    size_t nFeatures = NumericTable::cast((*dcPartialData)[0])->getNumberOfColumns();
+    if (NumericTable::cast((*dcPartialData)[0])->getNumberOfRows() != 0)


It is reasonable to put this check inside loop and just skip tables with 0 rows.

vepifanov · 2020-06-23T14:06:08Z

cpp/daal/src/algorithms/dbscan/dbscan_distributed_input_types.cpp

-    size_t nFeatures = 0;
-    for (size_t i = 0; i < nDataBlocks; i++)
+    size_t nFeatures = NumericTable::cast((*dcPartialData)[0])->getNumberOfColumns();
+    if (NumericTable::cast((*dcPartialData)[0])->getNumberOfRows() != 0)


it is reasonable to put this check inside loop and just skip tables with 0 rows.

vepifanov · 2020-06-23T14:06:35Z

cpp/daal/src/algorithms/dbscan/dbscan_distributed_input_types.cpp

-    size_t nFeatures = 0;
-    for (size_t i = 0; i < nDataBlocks; i++)
+    size_t nFeatures = NumericTable::cast((*dcPartialData)[0])->getNumberOfColumns();
+    if (NumericTable::cast((*dcPartialData)[0])->getNumberOfRows() != 0)


it is reasonable to put this check inside loop and just skip tables with 0 rows.

vepifanov · 2020-06-23T14:07:01Z

cpp/daal/src/algorithms/dbscan/dbscan_distributed_input_types.cpp

@@ -1197,14 +1198,17 @@ services::Status DistributedInput<step12Local>::check(const daal::algorithms::Pa
    const size_t nQueriesBlocks = dcOrders->size();
    DAAL_CHECK_EX(nQueriesBlocks > 0, ErrorIncorrectNumberOfInputNumericTables, ArgumentName, step12PartialOrdersStr());

-    for (size_t i = 0; i < nQueriesBlocks; i++)
+    if (NumericTable::cast((*dcOrders)[0])->getNumberOfRows() != 0)


it is reasonable to put this check inside loop and just skip tables with 0 rows.

vepifanov · 2020-06-23T14:07:40Z

cpp/daal/src/algorithms/dbscan/dbscan_partial_result_types.cpp


    const int unexpectedLayouts = (int)packed_mask;

-    for (size_t i = 0; i < nBlocks; i++)
+    if (NumericTable::cast((*dcPartitionedData)[0])->getNumberOfRows() != 0)


it is reasonable to put this check inside loop and just skip tables with 0 rows.

vepifanov · 2020-06-23T14:08:35Z

cpp/daal/src/algorithms/dbscan/dbscan_partial_result_types.cpp

@@ -152,26 +152,35 @@ Status DistributedPartialResultStep4::check(const daal::algorithms::Input * inpu
    DataCollectionPtr dcPartitionedPartialOrders = get(partitionedPartialOrders);

    DAAL_CHECK_EX(dcPartitionedData.get(), ErrorNullPartialResult, ArgumentName, partitionedDataStr());
-    DAAL_CHECK_EX(dcPartitionedData->size() == nBlocks, ErrorIncorrectDataCollectionSize, ArgumentName, partitionedDataStr());


You can also add check for nBlocks == 0 and skip all further checks in such case.

vepifanov · 2020-06-23T14:09:32Z

samples/daal/cpp/mpi/sources/dbscan_dense_distributed_mpi.cpp

@@ -184,6 +184,11 @@ void geometricPartitioning()
        sendCollectionAllToAll(beginId, endId, rankId, step4ResultPartitionedDataTag, curPartitionedData, partitionedData);
        sendCollectionAllToAll(beginId, endId, rankId, step4ResultPartitionedPartialOrdersTag, curPartitionedPartialOrders, partitionedPartialOrders);

+        if (partitionedData->size() == 0)


This workaround can be avoided using checks for nBlocks == 0.

I think it should be avoided. Because this code sequence makes sample less clear for the end user. @Pahandrovich please add nBlocks == 0 checks inside daal and remove those lines of code from the sample.

Vika-F

I suggest to remove changes in MPI sample. As samples should be as clear/simple as possible. You'll need to add checks for nBlocks == 0 to do so.

I'd also suggest to remove code duplications and make other changes listed in the comments.

Vika-F · 2020-06-25T13:26:40Z

cpp/daal/src/algorithms/dbscan/dbscan_dense_default_distr_impl.i

@@ -183,6 +183,16 @@ Status DBSCANDistrStep3Kernel<algorithmFPType, method, cpu>::compute(const DataC

    DAAL_OVERFLOW_CHECK_BY_MULTIPLICATION(size_t, totalNRows, sizeof(algorithmFPType));

+    if (totalNRows == 0)


It's better to move this if() before the overflow check.

Vika-F · 2020-06-25T13:40:40Z

cpp/daal/src/algorithms/dbscan/dbscan_distributed_input_types.cpp

-    size_t nFeatures = 0;
-    for (size_t i = 0; i < nDataBlocks; i++)
+    size_t nFeatures = NumericTable::cast((*dcPartialData)[0])->getNumberOfColumns();
+    if (NumericTable::cast((*dcPartialData)[0])->getNumberOfRows() != 0)
    {


This piece of code duplicates the pieces at lines 132, 256, 660. Please consider removing those duplicates.

Vika-F · 2020-06-25T13:42:09Z

cpp/daal/src/algorithms/dbscan/dbscan_distributed_input_types.cpp

-    size_t nFeatures = 0;
-    for (size_t i = 0; i < nDataBlocks; i++)
+    size_t nFeatures = NumericTable::cast((*dcPartialData)[0])->getNumberOfColumns();
+    if (NumericTable::cast((*dcPartialData)[0])->getNumberOfRows() != 0)


Vika-F · 2020-06-25T13:45:36Z

cpp/daal/src/algorithms/dbscan/dbscan_partial_result.h

@@ -179,7 +179,14 @@ DAAL_EXPORT services::Status DistributedPartialResultStep6::allocate(const daal:

    services::Status status;

-    set(step6ClusterStructure, HomogenNumericTable<int>::create(4, nRows, NumericTable::doAllocate, &status));
+    if (nRows != 0)


This if() statement looks meaningless to me. Why not revert to just
set(step6ClusterStructure, HomogenNumericTable::create(4, nRows, NumericTable::doAllocate, &status));
?

The same comment for other similar ifs in step8, step10 and step11.

I think the issue is that creating table with 0 rows and doAllocate flag leads to error raised.

Vika-F · 2020-06-25T13:54:22Z

samples/daal/cpp/mpi/sources/dbscan_dense_distributed_mpi.cpp

@@ -184,6 +184,11 @@ void geometricPartitioning()
        sendCollectionAllToAll(beginId, endId, rankId, step4ResultPartitionedDataTag, curPartitionedData, partitionedData);
        sendCollectionAllToAll(beginId, endId, rankId, step4ResultPartitionedPartialOrdersTag, curPartitionedPartialOrders, partitionedPartialOrders);

+        if (partitionedData->size() == 0)


I think it should be avoided. Because this code sequence makes sample less clear for the end user. @Pahandrovich please add nBlocks == 0 checks inside daal and remove those lines of code from the sample.

Vika-F · 2020-06-25T14:01:25Z

cpp/daal/src/algorithms/dbscan/dbscan_dense_default_distr_impl.i

    for (size_t part = 0; part < nBlocks; part++)
    {
        NumericTablePtr ntPartialSplit = NumericTable::cast((*dcPartialSplits)[part]);
        ReadRows<algorithmFPType, cpu> partialSplitRows(ntPartialSplit.get(), 0, 1);
        DAAL_CHECK_BLOCK_STATUS(partialSplitRows);
        const algorithmFPType * const partialSplit = partialSplitRows.get();

-        partialSplitValues[part] = partialSplit[0];
+        if ((size_t)partialSplit[1] == -1)


Consider re-writing this if(). It has hard-to-understand comparison of unsigned size_t value to a signed const -1. I'd rewrite to something like:
const size_t noSplitDim = -1;
...
if ((size_t)partialSplit[1] == noSplitDim)

fix distr dbscan with empty partial results

0af58c3

Pahandrovich requested review from SmirnovEgorRu, Vika-F and vepifanov June 17, 2020 22:21

Pahandrovich added 2 commits June 18, 2020 12:23

fix clang format

4ddc53a

fix clang-format 2

d5e4fd3

vepifanov reviewed Jun 23, 2020

View reviewed changes

Vika-F reviewed Jun 25, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix distributed dbscan with empty partial results #657

Fix distributed dbscan with empty partial results #657

Pahandrovich commented Jun 17, 2020

Pahandrovich commented Jun 18, 2020

vepifanov Jun 23, 2020

vepifanov Jun 23, 2020

vepifanov Jun 23, 2020

vepifanov Jun 23, 2020

vepifanov Jun 23, 2020

vepifanov Jun 23, 2020

vepifanov Jun 23, 2020

vepifanov Jun 23, 2020

vepifanov Jun 23, 2020

vepifanov Jun 23, 2020

Vika-F Jun 25, 2020

Vika-F left a comment

Vika-F Jun 25, 2020

Vika-F Jun 25, 2020

Vika-F Jun 25, 2020

Vika-F Jun 25, 2020

vepifanov Jun 25, 2020

Vika-F Jun 25, 2020

Vika-F Jun 25, 2020

		@@ -129,21 +129,23 @@ services::Status DistributedInput<step2Local>::check(const daal::algorithms::Par
		const size_t nBlocks = dcPartialData->size();
		DAAL_CHECK_EX(nBlocks > 0, ErrorIncorrectNumberOfInputNumericTables, ArgumentName, partialDataStr());

		@@ -249,19 +251,17 @@ services::Status DistributedInput<step3Local>::check(const daal::algorithms::Par
		const size_t nDataBlocks = dcPartialData->size();
		DAAL_CHECK_EX(nDataBlocks > 0, ErrorIncorrectNumberOfInputNumericTables, ArgumentName, partialDataStr());

		@@ -183,6 +183,16 @@ Status DBSCANDistrStep3Kernel<algorithmFPType, method, cpu>::compute(const DataC

		DAAL_OVERFLOW_CHECK_BY_MULTIPLICATION(size_t, totalNRows, sizeof(algorithmFPType));

		if (totalNRows == 0)

Fix distributed dbscan with empty partial results #657

Are you sure you want to change the base?

Fix distributed dbscan with empty partial results #657

Conversation

Pahandrovich commented Jun 17, 2020

Pahandrovich commented Jun 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vika-F 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