Added comments to methods implementation by goruha · Pull Request #41 · intervals-mining-lab/foapy

goruha · 2024-09-22T09:48:03Z

What

Added comments to the implementation alphabet method
Added comments to the implementation intervals method
Added comments to the implementation order method

coveralls · 2024-09-22T09:51:46Z

Pull Request Test Coverage Report for Build 12099465068

Details

1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.06%) to 94.779%

Totals
Change from base Build 11987743049:	-0.06%
Covered Lines:	236
Relevant Lines:	249

💛 - Coveralls

ChainsManipulator · 2024-09-23T04:16:31Z

src/foapy/alphabet.py

            {"message": f"Incorrect array form. Expected d1 array, exists {data.ndim}"}
        )
-
+    # Sort data positions


Maybe it would be clearer to write "Sort element indices" or "Get original indices of sorted data array"?

@ChainsManipulator Indices that would sort an array?
https://numpy.org/doc/stable/reference/generated/numpy.argsort.html

ChainsManipulator · 2024-09-23T04:25:01Z

src/foapy/alphabet.py


-    mask_shape = data.shape
-    unique_mask = np.empty(mask_shape, dtype=bool)
+    # Create tmp mask array to store True on positions where appears new value


"Create mask array to store True on positions where new value appears for the first time in the sorted array to distinguish where subarray of one element ends and another begins"

ChainsManipulator · 2024-09-23T05:01:28Z

src/foapy/alphabet.py

+    # unique_mask                       = [True, False, True, False, True, True]
+    #                                        a     a     c      c      d     e
+    unique_mask = np.empty(data.shape, dtype=bool)
+    # First element is new


"First element is always new"

ChainsManipulator · 2024-09-23T05:08:13Z

src/foapy/alphabet.py

+    # Set true on positions where value differs from previous
    unique_mask[1:] = data[perm[1:]] != data[perm[:-1]]

+    # Create tmp array that will store reverse sorted mask array


"Create mask array to store True on positions of the data array where new value appears for the first time"

ChainsManipulator · 2024-09-23T05:12:42Z

src/foapy/alphabet.py

-    result_mask[:1] = True
    result_mask[perm[unique_mask]] = True
+
+    # Return elements that are first appears of unique values


"Return array of first occurrences of elements in the data array"

ChainsManipulator · 2024-09-23T05:46:34Z

src/foapy/order.py

+    # Set true on positions where value differs from previous
    unique_mask[1:] = data[perm[1:]] != data[perm[:-1]]

+    # Create tmp array that will store reverse sorted mask array


"Create mask array to store True on positions of the data array where new value appears for the first time"

ChainsManipulator · 2024-09-23T05:49:11Z

src/foapy/order.py

+    # perm              = [    0,     5,     1,     2,     4,     3]
+    # perm[unique_mask] = [    0,            1,            4,     3]
+    # result_mask       = [True,   True, False,  True,  True, False]
+    #                       a      c       c       e      d     a


I would have moved this one line higher and added that this is "data" array

The difference is that on top in sorted data on bottom original order data

ChainsManipulator · 2024-09-23T05:49:39Z

src/foapy/alphabet.py

+    # perm              = [   0,      5,   1,     2,    4,    3]
+    # perm[unique_mask] = [   0,           1,           4,    3]
+    # result_mask       = [True, True, False, True, True, False]
+    #                        a     c     c      e      d     a


I would have moved this one line higher and added that this is "data" array

The difference is that on top in sorted data on bottom original order data

ChainsManipulator · 2024-09-23T06:36:06Z

src/foapy/intervals.py

    mask[1:-1] = ar[perm[1:]] != ar[perm[:-1]]
    mask[-1:] = True  # or  mask[-1] = True

+    # Save masks first and last appears of elements


"Create masks of first and last occurrences of elements by excluding first and last elements from unique_mask accordingly"

ChainsManipulator · 2024-09-23T06:48:06Z

src/foapy/order.py

+    #                           a  c  c  e  d  a
    inverse_perm[perm] = np.arange(data.shape[0])

+    # Create result array that count unique values starting from 0.


" Fill tmp result array with cumulative sums of number of unique values (starting from 0) to represent each element as a number in resulting order."

github-actions · 2024-09-24T09:54:34Z

Change	Before [`627f151`]	After [`fbd35c6`]	Ratio	Benchmark (Parameter)
-	4.55±0.05ms	3.72±0.1ms	0.82	bench_alphabet.AlphabetSuite.time_alphabet(500000, 'Best')

goruha · 2024-09-24T16:45:09Z

src/foapy/alphabet.py

            {"message": f"Incorrect array form. Expected d1 array, exists {data.ndim}"}
        )
-
+    # Sort data positions


@ChainsManipulator Indices that would sort an array?
https://numpy.org/doc/stable/reference/generated/numpy.argsort.html

src/foapy/alphabet.py

Co-authored-by: Igor Rodionov <goruha@gmail.com>

ChainsManipulator · 2024-11-28T08:50:39Z

src/foapy/alphabet.py

            {"message": f"Incorrect array form. Expected d1 array, exists {data.ndim}"}
        )
-
+    # Sort data positions


Suggested change

# Sort data positions

# Indices that would sort data array

ChainsManipulator · 2024-11-28T08:52:37Z

src/foapy/alphabet.py

+    # Set true on positions where value differs from previous
    unique_mask[1:] = data[perm[1:]] != data[perm[:-1]]

+    # Create tmp array that will store reverse sorted mask array


Suggested change

# Create tmp array that will store reverse sorted mask array

# Create mask array to store True on positions of the data array where new value appears for the first time

ChainsManipulator · 2024-11-28T08:55:43Z

src/foapy/alphabet.py

-    result_mask[:1] = True
    result_mask[perm[unique_mask]] = True
+
+    # Return elements that are first appears of unique values


Suggested change

# Return elements that are first appears of unique values

# Return array of first occurrences of elements in the data array

ChainsManipulator · 2024-11-28T08:56:20Z

src/foapy/intervals.py

+        # ar = ['a', 'd', 'e', 'c', 'c', 'a']
        ar = ar[::-1]

+    # Sort data positions


Suggested change

# Sort data positions

# Get original indices of sorted data array

ChainsManipulator · 2024-11-28T08:56:48Z

src/foapy/intervals.py

+    # perm = [0, 5, 1, 2, 4, 3]
    perm = ar.argsort(kind="mergesort")

+    # Create tmp mask array to store True on positions where appears new value.


Suggested change

# Create tmp mask array to store True on positions where appears new value.

# Create mask array to store True on positions where new value appears for the first time in the sorted array to distinguish where subarray of one element ends and another begins

ChainsManipulator · 2024-12-01T12:39:16Z

src/foapy/intervals.py

+        # result                  = [1, 2, 1, 4, 5, 5]
        result = intervals[inverse_perm]
    elif mod == mode.cycle:
+        # For cycle mode we permute intervals array to the original order


Suggested change

# For cycle mode we permute intervals array to the original order

# For cycle mode we permute intervals array to the original arrangement

ChainsManipulator · 2024-12-01T12:41:17Z

src/foapy/intervals.py

+        # result                  = [1, 2, 1, 4, 5, 5]
+
+        # Create 2-dimensional array size of (2, len(ar))
+        # Zero row is for intervals the first appearance of the element and intervals


Suggested change

# Zero row is for intervals the first appearance of the element and intervals

# Zero row is for the intervals of the first appearance of the element and intervals

ChainsManipulator · 2024-12-01T12:41:40Z

src/foapy/intervals.py

+        # Create 2-dimensional array size of (2, len(ar))
+        # Zero row is for intervals the first appearance of the element and intervals
+        # for intermediate appearances
+        # First row will store intervals for the last appearance of the element


Suggested change

# First row will store intervals for the last appearance of the element

# First row will store only intervals for the last appearance of the elements

ChainsManipulator · 2024-12-01T12:42:06Z

src/foapy/intervals.py

+        #                             ]
        result[last_mask, 1] = len(ar) - perm[last_mask]
+
+        # Permute intervals array to the original order


Suggested change

# Permute intervals array to the original order

# Permute intervals array to the original arrangement

ChainsManipulator · 2024-12-01T12:42:35Z

src/foapy/intervals.py

        result = result[result != 0]

    if bind == binding.end:
+        # For binding to the end, we need to reverse the result


Suggested change

# For binding to the end, we need to reverse the result

# For binding to the end, we need to reverse the result back

Added comments to foa methods implementation

524445d

goruha requested review from ChainsManipulator and masha-mtc September 22, 2024 09:48

masha-mtc previously approved these changes Sep 22, 2024

View reviewed changes

goruha requested a review from masha-mtc September 22, 2024 09:51

ChainsManipulator requested changes Sep 23, 2024

View reviewed changes

Merge branch 'main' into comment-foa-implementations

a640eb6

goruha commented Sep 24, 2024

View reviewed changes

src/foapy/alphabet.py Outdated Show resolved Hide resolved

Apply suggestions from code review

283f478

Co-authored-by: Igor Rodionov <goruha@gmail.com>

ChainsManipulator dismissed masha-mtc’s stale review via 283f478 November 28, 2024 09:25

goruha added 3 commits November 28, 2024 16:17

Merge branch 'main' into comment-foa-implementations

14db979

Update alphabet.py

e7b52bd

Address comments

0926453

ChainsManipulator reviewed Dec 1, 2024

View reviewed changes

	# Create tmp array that will store reverse sorted mask array
	# Create mask array to store True on positions of the data array where new value appears for the first time

	# Return elements that are first appears of unique values
	# Return array of first occurrences of elements in the data array

	# Sort data positions
	# Get original indices of sorted data array

	# Create tmp mask array to store True on positions where appears new value.
	# Create mask array to store True on positions where new value appears for the first time in the sorted array to distinguish where subarray of one element ends and another begins

	# For cycle mode we permute intervals array to the original order
	# For cycle mode we permute intervals array to the original arrangement

	# Zero row is for intervals the first appearance of the element and intervals
	# Zero row is for the intervals of the first appearance of the element and intervals

	# First row will store intervals for the last appearance of the element
	# First row will store only intervals for the last appearance of the elements

	# Permute intervals array to the original order
	# Permute intervals array to the original arrangement

	# For binding to the end, we need to reverse the result
	# For binding to the end, we need to reverse the result back

Conversation

goruha commented Sep 22, 2024

What

Uh oh!

coveralls commented Sep 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 12099465068

Details

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Sep 22, 2024 •

edited

Loading