6
\$\begingroup\$

I have a part of code that is loading a dataset and normalizing property values to [0, 1]. My implementation is:

import pickle
import numpy as np

# -- load data
prop_1    = list(np.random.rand(10)*20)
prop_2    = list(np.random.rand(10)*10)
prop_3    = list(np.random.rand(10)*30)

# -- normalize
l_bound = []
l_bound.append(min(prop_1))
l_bound.append(min(prop_2))
l_bound.append(min(prop_3))
u_bound = []
u_bound.append(max(prop_1))
u_bound.append(max(prop_2))
u_bound.append(max(prop_3))

prop_1 = (np.array(prop_1) - l_bound[0]) / (u_bound[0] - l_bound[0])
prop_2 = (np.array(prop_2) - l_bound[1]) / (u_bound[1] - l_bound[1])
prop_3 = (np.array(prop_3) - l_bound[2]) / (u_bound[2] - l_bound[2])

However, the normalizing part of the code does not look graceful. Any suggestions on how to improve it? Can do this using a loop?

\$\endgroup\$
  • \$\begingroup\$ Can you show the result of prop_* as loaded by pickle? \$\endgroup\$ – Reinderien Mar 30 at 1:41
  • \$\begingroup\$ Also, why are you loading the same file three times? \$\endgroup\$ – Reinderien Mar 30 at 1:41
  • \$\begingroup\$ It's not the same file, it's 3 separate variables. Each variable is a list of property values for 2000 molecules. \$\endgroup\$ – Blade Mar 30 at 1:46
  • \$\begingroup\$ It... definitely looks like the same file to me. You use f three times. What am I missing? \$\endgroup\$ – Reinderien Mar 30 at 1:47
  • \$\begingroup\$ That's just how pickle works, you save data using: with open(dataset_name, "wb") as f: pickle.dump(prop_1, f) pickle.dump(prop_2, f) pickle.dump(prop_3, f) \$\endgroup\$ – Blade Mar 30 at 1:49
5
\$\begingroup\$

Since you have numpy arrays, you should use their vectorized methods wherever possible. This can make your code a lot faster:

In [1]: x = np.arange(10000000)

In [2]: %timeit max(x)
988 ms ± 42.6 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

In [3]: %timeit x.max()
9.67 ms ± 114 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

This includes not casting your arrays to list.

I would also make this a function that normalizes a single array:

import pickle
import numpy as np
from typing import Iterable, Any

def normalize_one(x: Iterable[Any]) -> np.ndarray:
    if not isinstance(x, np.ndarray):
        x = np.array(list(x))
    low, diff = x.min(), x.ptp()
    return (x - low) / diff

# -- load data
prop_1    = np.random.rand(10)*20
prop_2    = np.random.rand(10)*10
prop_3    = list(np.random.rand(10)*30

# -- normalize
prop_1 = normalize_one(prop_1)
prop_2 = normalize_one(prop_2)
prop_3 = normalize_one(prop_3)

If you do have many arrays that need to be normalized, you can always do it in a list comprehension:

properties = [prop_1, prop_2, prop_3]
properties = [normalize_one(prop) for prop in properties]

If you have many of them and they all have the same structure, I would use something like this (now limited to numpy arrays as input):

def normalize(x: np.ndarray, axis: int = 1) -> np.ndarray:
    """Normalize the array to lie between 0 and 1.
    By default, normalizes each row of the 2D array separately.
    """
    low, diff = x.min(axis=axis), x.ptp(axis=axis)
    # Indexing needed to help numpy broadcasting
    return (x - low[:,None]) / diff[:,None]

properties = np.random.rand(3, 10)
properties[0] *= 20
properties[1] *= 10
properties[2] *= 30

properties = normalize(properties)

For props = np.random.rand(10000, 10) I get the following timings:

Author      Timed function call                       Time [s]
Blade*      list(normalize_blade(props))              68.7 ms ± 749 µs
Linny       list(normalize_linny(*props))             127 ms ± 1.42 ms
Graipher    [normalize_one(prop) for prop in props]   119 ms ± 7.4 ms
Graipher    normalize(props)                          2.32 ms ± 113 µs

The code I used for the test with the code in the OP is this one, which is just the generalization to many properties:

def normalize_blade(properties):
    l_bound, u_bound = [], []
    properties = [list(prop) for prop in properties]
    for prop in properties:
        l_bound.append(min(prop))
        u_bound.append(max(prop))
    for i, prop in enumerate(properties):
        yield (np.array(prop) - l_bound[i]) / (u_bound[i] - l_bound[i])
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ strange that normalize_one is slower than normalize_blade, using the list appends and builtin min and max instead of the numpy methods \$\endgroup\$ – Maarten Fabré Mar 30 at 11:58
  • \$\begingroup\$ @MaartenFabré I agree, I was also surprised. It might be due to the isinstance, but I'm not sure. Or maybe numpy.ptp is slower than numpy.max plus one subtraction. Also, note that each property is only 10 elements long, as in the OP, so the built-in operations are still quite fast. If you see any obvious mistakes in my timing, let me know. \$\endgroup\$ – Graipher Mar 30 at 12:22
  • \$\begingroup\$ I don't see why it's slower either. If you write it as ` low, high = x.min(), x.max() return (x - low) / (high - low)` instead with ptp is about 25% faster, but still slower than OP. Removing the isinstance and list speeds it up a bit, but still not enough \$\endgroup\$ – Maarten Fabré Mar 30 at 14:59
4
\$\begingroup\$

keep your raw data

You overwrite props_x with the normalized version. Better would be to make this a new variable

data structures

If you have more than 1 or 2 properties. Assigning them each to their own variable can become quite tedious. You need to gather them in a data structure. If they are all of the same length, a numpy.array or pandas.DataFrame can be the right structures. Otherwise a dict might be more appropriate

data_raw = {
    "prop_1": list(np.random.rand(10) * 20),
    "prop_2": list(np.random.rand(10) * 10),
    "prop_3": list(np.random.rand(10) * 30),
}

function

Almost each time you write a comment in your code denoting a section, you can make the code itself clearer by putting that section in a data structure, function, class, ...

def normalize(iterable: Iterable[Any]) -> np.array:
    """Linearly scales the input to the interval [0, 1]"""
    my_array = np.array(list(iterable))
    lower, upper = my_array.min(), my_array.max()
    return (my_array - lower) / (upper - lower)

I even added a docstring explaining what the method does.

data_normalized = {
    name: normalize(data)
    for name, data in data_raw.items()
}

spacing

For code formatting, I trust black to make the correct choices for me.So no more prop_1 =, but 1 space around the =, ...

The only coniguration I use there is maximum 79 character per line.

Black integrates wellwith most IDEs and jupyter lab notebooks (docs)

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

I took a stab at this, not really knowing what you wanted. The function below yields the outcome of each calculation made in your program. You can pass any amount of np.array to it, aka any iterables, and it will make the calculation based on what you passed. Have a look:

from typing import Iterable, Any
import numpy as np

def normalize(*iterables: Iterable[Any]) -> Iterable[np.array]:
    for iterable in iterables:
        my_array = np.array(list(iterable))
        lower, upper = my_array.min(), my_array.max()
        yield (my_array - lower) / (upper - lower)

Thanks to @Maarten Fabré for pointing out that real iterables were excluded from this program, and would fail. It now works with these, as displayed below. This function also now complies with PEP 0484 type hints regarding iterables.

Here's how you could use this:

props = [list(np.random.rand(10)*20), list(np.random.rand(10)*10), list(np.random.rand(10)*30)]

for prop in normalize(array for array in props):
    print(prop)

for prop in normalize(props):
    print(prop)

I also tested the efficiency of your program against this one.

print(f"OLD: {timeit.timeit(old_normalize, number=100000)} seconds.")
print(f"NEW: {timeit.timeit(normalize, number=100000)} seconds.")
OLD: 2.7710679 seconds.
NEW: 0.0201071 seconds.
\$\endgroup\$
  • \$\begingroup\$ your method would not work on a read iterable. Did you try list(normalize((i for i in range(3)))). You need to create the array first, and then take the min and max from that. That will be a lot faster for largers arrays too \$\endgroup\$ – Maarten Fabré Mar 30 at 7:08
  • \$\begingroup\$ mypy also complains about the type annotations. It should be def normalize(*iterables: Iterable[Any]) or iterable[float] if you want to limit the operation to numbers \$\endgroup\$ – Maarten Fabré Mar 30 at 7:14
  • 1
    \$\begingroup\$ That's not what I meant. Calling it with an iterable still will not work... you need something like this: def normalize(*iterables: Iterable[Any]) -> np.array: for iterable in iterables: my_array = np.array(list(iterable)) lower, upper = my_array.min(), my_array.max() yield (my_array - lower) / (upper - lower) \$\endgroup\$ – Maarten Fabré Mar 30 at 7:43
  • 1
    \$\begingroup\$ @MaartenFabré That makes a lot more sense. Thanks so much for the clarification. I'll community wiki this since you helped greatly in the process of fixing this answer. \$\endgroup\$ – Linny Mar 30 at 8:02
  • 1
    \$\begingroup\$ Are you sure you included a list call in your timing? Because otherwise it is just the time needed to setup the generator. \$\endgroup\$ – Graipher Mar 30 at 8:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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