Faster isin comparisons #7

TomAugspurger · 2018-02-21T19:36:23Z

No description provided.

martindurant

I must admit I am confused by what is here.
Please forgive my ignorance if I've got everything back-to-front.

I note that the network objects could be addressed as n.network_address.packed and n.netmask.packed to get 8- or 16-byte byte-strings which could be ingested by numpy.

martindurant · 2018-02-21T20:36:48Z

cyberpandas/ip_array.py

-            for i, ip in enumerate(pyips):
-                if ip in network:
-                    mask[i] = True
+            mask |= self._isin_network(network)


Is there no way to short-circuit here? Or is that something for a future cythonized method...

Why loop to create networks and then loop again?

Unless I'm missing something, I don't think there's anything to short-circut. We may not know whether an element of self is in any of the other networks until the last network.

Some context, I expected self to be large and networks to be small. So I'm OK looping over networks.

martindurant · 2018-02-21T20:38:45Z

cyberpandas/ip_array.py

        return mask

+    def _isin_network(self, other):
+        # type: (Union[ipaddress.IPv4Network,ipaddress.IPv6Network]) -> ndarray


Return type here is bool, no?

This is elementwise, like pandas.Seres.isin

In [9]: pd.Series(['a', 'b', 'c']).isin(['a', 'b']) Out[9]: 0 True 1 True 2 False dtype: bool

martindurant · 2018-02-21T20:43:40Z

cyberpandas/ip_array.py

+        net_hi = int(other.broadcast_address)
+
+        if isinstance(other, ipaddress.IPv4Network):
+            # We know we only need the lower 64 bits


Implicitly an IP4 address (where the hi part is non-zero) cannot be in an IP6 address?

Correct. The largest IPv4 address is

In [13]: IPArray([2 ** 32 - 1]).data Out[13]: array([(0, 4294967295)], dtype=[('hi', '>u8'), ('lo', '>u8')])

So being IPv4 implies the hi bit is 0 (the inverse is not true, since some IPv6 addresses are less than 2 ** 64 -1).

martindurant · 2018-02-21T20:46:41Z

cyberpandas/ip_array.py

        return mask

+    def _isin_network(self, other):


Should also accept list/array of IPs rather than networks?

My plan is for isin to accept either

A single network / network string like '192.168.0.0/28', or a list like of those

A list of IP Addresses

The first would call into this method. The second would call into an _is_ipaddresses(self, addresses) method that hasn't been written yet..

martindurant · 2018-02-21T20:55:32Z

cyberpandas/ip_array.py

+        if isinstance(other, ipaddress.IPv4Network):
+            # We know we only need the lower 64 bits
+            return (self.data['hi'] == 0) & ((net_lo <= self.data['lo']) &
+                                             (self.data['lo'] <= net_hi))


This doesn't entirely look like the condition (self.data['lo'] & int(other.netmask)) == int(other.network) from the linked SO article.

I should update that comment. I changed it to use what IPv4Network.__contains__ does.

# dealing with another address else: # address return (int(self.network_address) <= int(other._ip) <= int(self.broadcast_address))

Which I think is equivalent to the bit masking linked in the SO article.

martindurant · 2018-02-21T21:09:46Z

cyberpandas/ip_array.py

+                hi_mask = (self.data['hi'] > 0) & hi_mask_lo & hi_mask_hi
+                # This seems incorrect...
+                lo_mask = self.data['lo'] < (net_hi - net_lo)
+                return hi_mask & lo_mask


Again, this maybe should be

(self.data['lo'] & int(other.netmask) & lo_mask) == (int(other.network) & lo_mask) & ( (self.data['hi'] & int(other.netmask)>>64) == (int(other.network)>>64)

(lo_mask is 0xffffffff).

Did you mean

(self.data['lo'] & int(other.netmask) & lo_mask) == (int(other.network_address) & lo_mask) & ( (self.data['hi'] & int(other.netmask)>>64) == (int(other.network_address)>>64))

with .network_address instead of network? That faills the test for

s = ip.IPArray([u'2001:db8::1', u'2001:db9::1']) result = s.isin([u'2001:db8::0/96'])

It says both are in the network, while '2001:db9::1' should not be (I think).

I'll take a closer look at your approach though. This stuff is confusing for me :/

TomAugspurger · 2018-02-22T12:42:17Z

I forgot that I had already written code for comparisons of IPArrays. Much simpler now.

martindurant · 2018-02-22T15:32:41Z

cyberpandas/ip_array.py

+        # above by 'broadcast_address'.
+        # IPArray handles comparisons between arrays of addresses, and NumPy
+        # handles broadcasting.
+        net_lo = type(self)([other.network_address])


Will this fail on attempt to compare IP4 and IP6?

Once we're in an IPArray, we don't care about IPv4 vs. IPv6. Everything is 128 bits.

martindurant · 2018-02-22T15:36:22Z

cyberpandas/ip_array.py

+        net_lo = type(self)([other.network_address])
+        net_hi = type(self)([other.broadcast_address])
+
+        return (net_lo <= self) & (self <= net_hi)


Is this using ipaddress's internal comparisons, or is this implemented by us elsewhere? I like that it ended up as a simple one-liner.

Implemented by us in

cyberpandas/cyberpandas/ip_array.py

Line 185 in 52e28f7

def __lt__(self, other):

.

Amazing what a night of sleep can do to change your perspective on a problem :)

martindurant · 2018-02-22T15:47:50Z

OK, liking it.
For our own selves, should see how bad the pathological case of (oneIPaddress).ip.isin(bigIParray) is.

Faster isin comparisons

fd9c3d7

martindurant reviewed Feb 21, 2018

View reviewed changes

TomAugspurger added 2 commits February 21, 2018 16:16

Merge remote-tracking branch 'origin/master' into faster-isin

f1afbeb

Simplify

ae54396

martindurant reviewed Feb 22, 2018

View reviewed changes

TomAugspurger added 5 commits February 22, 2018 14:21

Updated docstring

f33dcea

Merge remote-tracking branch 'upstream/master' into faster-isin

14a8800

Support IPArray comparison

c524e74

Linting

e6a6666

Py2 compat

2fc7525

TomAugspurger merged commit 6b5d262 into master Feb 23, 2018

TomAugspurger deleted the faster-isin branch February 23, 2018 18:24

Faster isin comparisons #7

Faster isin comparisons #7

Uh oh!

Conversation

TomAugspurger commented Feb 21, 2018

Uh oh!

martindurant left a comment

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!

martindurant Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

TomAugspurger commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

martindurant commented Feb 22, 2018

Uh oh!

Uh oh!

martindurant Feb 21, 2018 •

edited

Loading

TomAugspurger commented Feb 22, 2018 •

edited

Loading