Added comments to methods implementation #41

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

    data = np.asanyarray(X)
    if data.ndim > 1:  # Checking for d1 array
        raise Not1DArrayException(
            {"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

    perm = data.argsort(kind="mergesort")

-    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

    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[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

    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_shape = ar.shape
    mask = np.empty(mask_shape[0] + 1, dtype=bool)
    mask[:1] = True
    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

    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

    data = np.asanyarray(X)
    if data.ndim > 1:  # Checking for d1 array
        raise Not1DArrayException(
            {"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

    data = np.asanyarray(X)
    if data.ndim > 1:  # Checking for d1 array
        raise Not1DArrayException(
            {"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

    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[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 = 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 = 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 = 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 masha-mtc and ChainsManipulator 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added comments to methods implementation #41

Added comments to methods implementation #41

goruha commented Sep 22, 2024

coveralls commented Sep 22, 2024 •

edited

Loading

ChainsManipulator Sep 23, 2024

goruha Sep 24, 2024

ChainsManipulator Sep 23, 2024

ChainsManipulator Sep 23, 2024

ChainsManipulator Sep 23, 2024

ChainsManipulator Sep 23, 2024

ChainsManipulator Sep 23, 2024

ChainsManipulator Sep 23, 2024

goruha Nov 30, 2024

ChainsManipulator Sep 23, 2024

goruha Nov 30, 2024

ChainsManipulator Sep 23, 2024

ChainsManipulator Sep 23, 2024

github-actions bot commented Sep 24, 2024

goruha Sep 24, 2024

ChainsManipulator Nov 28, 2024

ChainsManipulator Nov 28, 2024

ChainsManipulator Nov 28, 2024

ChainsManipulator Nov 28, 2024

ChainsManipulator Nov 28, 2024

ChainsManipulator Dec 1, 2024

ChainsManipulator Dec 1, 2024

ChainsManipulator Dec 1, 2024

ChainsManipulator Dec 1, 2024

ChainsManipulator Dec 1, 2024

		unique_mask[1:] = data[perm[1:]] != data[perm[:-1]]

		# Create tmp array that will store reverse sorted mask array

		result_mask[perm[unique_mask]] = True

		# Return elements that are first appears of unique values

		inverse_perm[perm] = np.arange(data.shape[0])

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

	# 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

		perm = ar.argsort(kind="mergesort")

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

	# 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

		result[last_mask, 1] = len(ar) - perm[last_mask]

		# Permute intervals array to the original order

	# 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 methods implementation #41

Are you sure you want to change the base?

Added comments to methods implementation #41

Conversation

goruha commented Sep 22, 2024

What

coveralls commented Sep 22, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12099465068

Details

💛 - Coveralls

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 24, 2024

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

coveralls commented Sep 22, 2024 •

edited

Loading