2
\$\begingroup\$

I have developed a class structure with methods to return a matrix from a single input array. The objective is to insert the middle value of the array so 2n remains in terms of array length. So there should be an even number always. The minimum length is provided by a prime number, which is not a product of two smaller numbers. Therefore, 5 should only have [1,5] as a list.

For example, if I want all divisors of the number 10 these are 1, 2, 5, 10, this is fine as its 2n in length. However, for an array with divisors of 16 I get 1, 2, 4, 8, 16 which is odd. The objective is to find all the divisors, then reduce the matrix when each divisor is found. The reduction is based on the outer 2 values, so [1, 16], [2,8], [4] but 4 on its own cannot make 16, so we insert 4 again to get [1, 2, 4, 4, 8, 16] which is 2(3) in length.

It's a little messy and so I wanted to receive a review on my code. It's my first time implementing a class with methods - I was thrilled to get this to work! I aim to implement logarithmic transformation and more in the future, so this was just the first step. It's very slow with numbers larger than 1,000,000

A few examples of the print output:

data = data_structure()
print(data.reshape_array(16),data.divisor(16),data.mid_value(16))
[[ 1 16] ,[ 1  2  4  8 16] ,[ 1  2  4  4  8 16]
 [ 2  8]
 [ 4  4]]

Here's what I have tried:

import numpy as np

"""
This should from any input create a numpy array and return a matrix that splits the array
into 2n parts. For odd array lengths, the mid-value is inserted to get an even value.
"""

class data_structure:

    def divisor(self,number):
        i = 0
        divisors=[]
        while i < number:
            i+=1
            for mult in range(0, number+1, i):
                if number == mult:
                    divisors.append(i)
        return np.asarray(divisors) #return the divisors as a numpy list
        #ADD A MID VALUE:

    def mid_value(self, number, no_array = False):
        self._mid_value = self.divisor(number) #set _mid_value as the divisor list
        if no_array == False:
            if len(self._mid_value) %2 != 0:
                mid_value = int((len(self._mid_value) - 1)/2)
                for ind, middle in enumerate(self._mid_value):
                    if middle == self._mid_value[mid_value]:
                        test=np.insert(self._mid_value, ind, middle)
                        return test
            else:
                return self._mid_value
        else:
            pass

    def reshape_array(self,number): 
        _array_ = self.mid_value(number, no_array = False).copy()
        array_ = self.mid_value(number, no_array = False)
        
        len_arr_ = len(array_)
        n = 0
        reduced_arrays = []
        if len_arr_ > 2:
            while n < len_arr_: # less than total length of list
                n += 2
                if len_arr_ > n: #if the length of the array is greater than 2 do conditional
                    splice = len_arr_-(n-1) #we want to splice the outer two values and delete then reduce
                    reduced_arrays.append(array_[::splice])
                    search_reduce = np.searchsorted(array_, array_[::splice])
                    reduce_array = np.delete(array_, search_reduce)
                    array_ = reduce_array #this is to return to the spliced value when reduced
                    if len(array_) == 2:
                        reduced_arrays.append(array_) #we do not get the reduced values above so get the value two values
                    else:
                        continue
        else:
            return _array_
        return np.column_stack(reduced_arrays).T   #this should stack them and return a matrix with maximum 2 columns and n+=1 rows
 
```
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Your exposition could use a good proof reader to make it a bit easier to follow. To adjust some mathematical terminology, "prime number" in your first paragraph seems to mean something else; the multiples of 10 are 10, 20, 30, 40, 50, ...; the divisors of 10 are 1, 2, 5, and 10; "reduce" seems to mean something else; and "one-to-one mapping from the natural numbers to 2n" seems to mean something else. \$\endgroup\$
    – Teepeemm
    May 20, 2022 at 2:12
  • \$\begingroup\$ @Teepeemm Thank you for the corrections! Math is not my background so I appreciate the support. I'll work on improving the sentences in the meantime \$\endgroup\$
    – Dollar X
    May 20, 2022 at 8:58
  • \$\begingroup\$ @Teepeemm I left out the mapping part altogether, as it would add unnecessary abstraction. I hope it is much clearer now! \$\endgroup\$
    – Dollar X
    May 20, 2022 at 9:07

1 Answer 1

1
\$\begingroup\$
  • PEP8 specifies CapWords for class names. But you should figure out a better class name than DataStructure, since that's not very descriptive.
  • Your docstring starts out "from any input". You really mean "for a positive number". The docstring should come immediately after the class definition, so that DataStructure.doc (and other Python stuff) can find it.
  • To my math-inclined mind, if you say something is "2n", I assume that "n" is related to something, which in this case must be the input. So I would change "2n" to "even length"
  • I'm not too concerned with long lines if they're unavoidable. But if it's just a comment that makes the line too long, then split that up a bit.
  • i=0;while i<number:i+=1 is a really complicated way of writing for i in range(1,number+1).
  • Add a __name__=='__main__' guard, so that you can incorporate the unit test you showed us. You should also test when number is prime (so you have a 2x1 output), and a non-repeated value non-prime number (which is the most common case).
  • Your approach for finding divisors is really strange. It looks like you're trying to avoid using the % operator. We can just check if number%i==0. The method is now short enough that we can put that into a list expression: divisors = [ i for i in range(1,number+1) if number%i==0 ].
  • else:pass is useless.
  • You want to use booleans without explicitly comparing them to True or False. if no_array == False should just be if not no_array. But now we have a double negative, so lets change no_array into want_array. But:
  • mid_value by default modifies the array and then returns it. If I toggle the flag the other way, it creates a private variable that nothing else accesses. That's pointless, so lets just remove that possibility.
  • In a similar idea of removing ==False, you could consider changing if len(_mid_value) %2 != 0 to if len(_mid_value) %2.
  • Since nothing else needs to access _mid_value, lets not add it to the class.
  • Instead of int((len(_mid_value) - 1)/2), we can just use len(_mid_value)//2, since we know the length is odd.
  • You're going through enumerate(_mid_value) to find a specific value at a specific index. You already know the index (and hence the value), so you don't need the for loop. (This is the second time you've used a for loop just to do an action for a single entry. Try to avoid that.)
  • The function mid_value has a variable mid_value. I'm going to rename the latter to mid_index.
  • If len_arr_<2, then _array_ and array_ are the same thing. We can return array_ and remove _array_. But in that case, you get a 1-d list, whereas with longer lists you get a 2-d list. We should change that so that you always get a 2-d list.
  • np.column_stack().T is the same as np.row_stack()
  • Don't add a comment if it's repeating what the code is doing. At best this is redundant and distracting (your first three comments). At worst, it's different than what the code says, leaving you wondering if the comment is out of date or actually useful ("if the length of the array is greater than 2 do conditional").
  • If mid_value overwrites _mid_value instead of assigning to test, then you can always return _mid_value and don't need to worry about the else.
  • Your function names are strange. divisor and mid_value sound like a single number, not a list of them. reshape_array sounds like it takes an array and reshapes it, not takes a number and returns the reshaped array. Instead, I'm going to rename divisor to get_divisors, mid_value_ to get_divisors_with_mid_value, and reshape_array to get_divisors_as_matrix.
  • divisor doesn't need self, so lets mark it as a @staticmethod. The only point of self in the other two is to be able to call another method of the class. So they can also become @staticmethod. But:
  • You're not really using anything as a class. It's just three related functions. Personally, I'd skip the class and just make them functions (especially if you're only ever really using the 2xn matrix form). But let's leave them as a class for instructional sake. In that case, it makes more sense to pass the number to the constructor and make the arrays as part of the initialization (this means the functions aren't static after all). We'll move all the functions into the initialization (they're not that long anymore), and create properties with getters.
  • search_reduce is the indices 0 and len-1. It should be better named, and you shouldn't use np.searchsorted when you know what the outcome will be. But that whole section is really convoluted. Since you know it's going to be the outer two values, you can just use array_[0] and array_[-1] and delete those (and because its numpy, we can take and delete in one command each). It's now generic enough that you don't need the if len_arr_ > 2 anymore: while array_.size: reduced_arrays.append(array_[[0,-1]]); array_ = np.delete(array_,[0,-1]).
  • Even better: we know that the first column will be the first half of the array, and the second column will be the last half of the array, but in the other order. Let's just do that.
  • Even better: if number is big, then you don't want to take too much time checking all the intermediate values for divisors. You can use a shortcut: when you find a divisor, you know that number/divisor is also a divisor. And if you've been looking from 1 onward, then number/divisor is already decreasing. (If number is really big, it will be better to find the divisors using the prime factorization of number, but that's a much more complicated topic.)
  • The middle value gets repeated when number is a perfect square. This doesn't really factor into my code, but it should be stated.

The result:

import numpy as np

class DataStructure:
    """
    For a positive number, this should create a numpy array of all divisors and
    return a matrix that splits the array into 2 parts. For odd array lengths,
    the mid-value is repeated to get an even length.
    """
    def __init__(self,number):
        _small_divisors = np.asarray([ i for i in range(1,int(number**.5)+1) if number%i==0 ])
        _large_divisors = number//_small_divisors
        self.divisors_with_mid_value = np.concatenate([_small_divisors,[*reversed(_large_divisors)]])
        self.divisors_as_matrix = np.column_stack([_small_divisors,_large_divisors])
        if ( _small_divisors[-1] == _large_divisors[-1] ):
            self.divisors = np.concatenate([_small_divisors[:-1],[*reversed(_large_divisors)]])
        else:
            self.divisors = self.divisors_with_mid_value

if __name__=='__main__':
    for number in (7,16,30):
        data = DataStructure(number)
        print(data.divisors_as_matrix,data.divisors,data.divisors_with_mid_value)
'''
[[1 7]] [1 7] [1 7]
[[ 1 16]
 [ 2  8]
 [ 4  4]] [ 1  2  4  8 16] [ 1  2  4  4  8 16]
[[ 1 30]
 [ 2 15]
 [ 3 10]
 [ 5  6]] [ 1  2  3  5  6 10 15 30] [ 1  2  3  5  6 10 15 30]
'''
\$\endgroup\$
2
  • \$\begingroup\$ Thank you for the in-depth review, I cannot believe how much faster yours runs! I will study your syntax in depth and explanation, I still have so much to learn in Python. \$\endgroup\$
    – Dollar X
    May 22, 2022 at 9:23
  • \$\begingroup\$ The main reason for the speed gain is probably that your divisor function is O(n^2) while the corresponding part of mine is O(√n). \$\endgroup\$
    – Teepeemm
    May 22, 2022 at 13:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.