Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix GeoPatch algorithm #264

Merged
7 commits merged into from
Apr 19, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 27 additions & 56 deletions lib/archethic/p2p/geo_patch.ex
Original file line number Diff line number Diff line change
Expand Up @@ -24,73 +24,44 @@ defmodule ArchEthic.P2P.GeoPatch do
end

defp compute_random_patch do
list_char = Enum.concat([?0..?9, ?A..?F])
Enum.take_random(list_char, 3) |> List.to_string()
list_char1 = Enum.concat([?0..?9, ?A..?F])
list_char2 = Enum.concat([?0..?3, ?C..?F])

Enum.take_random(list_char1, 2)
|> List.insert_at(1, Enum.take_random(list_char2, 1))
|> List.to_string()
end

defp compute_patch(lat, lon) do
lat_sign = sign(lat)
lon_sign = sign(lon)

fdc = [lat / 90, lon / 180]

sd =
[(lat - lat_sign * 45) / 2, (lon - lon_sign * 90) / 2]
|> resolve_with_sign([lat, lon])
lon_pos = (lon + 180) / 22.5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@I-Archer-Zero
Looks like added 90,180 to avoid negative sign but look at

lat_pos

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It must be
lat_pos = (lat + 90) / 22.5

Copy link
Member Author

@Neylix Neylix Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi !
Dividing lat by 22.5 will have a result between 0 and 8. But we are looking for a range between 0 and 16 to get all hexadecimal possibilities (0 to F).
So we have to divide by 11.25 to get it.

I think on issue's example there is a mismatch. On the graph, latitude only have 8 parts, but on explication bellow it says that first 2 digits are on range of 0 to F so 16 parts...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@I-Archer-Zero

Copy link
Member

@internet-zero internet-zero Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Neylix (Cc @samuel-uniris)
Good observation!

lat should be divided by 22.5 and not 11.25 because we need square patches (not rectangle)
This also means that the lat/22.5 will have a result between 0 to 8

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi ! Thanks for your return.
I didn't know it had to be a square, I'll update it as you requested. Also need to add 4 to result to get index from C to 3 as in your graph.
I also noticed that in issue you say that first digit is for longitude index, but I did latitude first, should I update it too ?
I'm actually out of my PC until sunday, I'll update it at this time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello !
I fixed calculation in lastest commits

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest you can you >= and <= condition checks and fix the edge cases and also implement the algo as mentioned in #253. So, first digit and third digt can be [8,9,A,B,C,D,E,F,0,1,2,3,4,5,6,7] And second digit can be [C,D,E,F,0,1,2,3]

As precision digit being computed as per previous implementation.

# Adding 4 to have second digit hex value from C to 3
lat_pos = (lat + 90) / 22.5 + 4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avoid adding 4 and adding 90, 180 was in earlier implementation so, avoid it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello !
I don't really understand your suggestion. Here is why I choose this implementation instead of the one did before :
The first calculations with adding 90 / 180 / 4 result in an index between 0 and 15 for lon_pos one and between 4 and 11 for lat_pos.
With these index, we can easily find the good hexadecimal value in the array of the function main_index_patch.

The advantage of this implementation is that it takes less ressources and less execution times than the previous implementation, i.e guetting a value from an array by his index is faster than doing multiple if condition to find the good hexadecimal value.
An other advantage is that it takes less lines of code.

To be easier to read, instead of adding 4 to lat_pos we can create a new array [C,D,E,F,0,1,2,3] and get hexadecimal value from it, but I think that using same array as for lon_pos is better to reduce memory usage.

@samuel-uniris What is your advice about it ?

Copy link

@ghost ghost Apr 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in term of performance pattern matching (multiple function clause) is like if/else block under the good bu is quite fast and optimized during compilation.

Accessing a random element in a list is not the same in Erlang. List are not arrays but linked list , which means to access nth element, need to scan nth before . When the list is big is not efficient.
However if the list is bounded, know and static, you can leverage tuple where access a specific element is fast.

There is different strategy for this problem.

In term of readability the pattern matching is better when you have a several if/else/case blocks.

For reusability, sure it's better to keep a main indexing/reference, but keep in mind it's function programming, so variables a immutable. So you get a new memory reference after each mutation or assignment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi ! Updated in last commit to use tuple. Using pattern matching function was not as easy to read because it needs to much function to catch all conditions.


sdc = [List.first(sd) / 22.5, List.last(sd) / 45]
first_digit = main_index_patch(trunc(lon_pos))
second_digit = main_index_patch(trunc(lat_pos))

td =
[
(List.first(sd) - lat_sign * 11.25) / 2,
(List.last(sd) - lon_sign * 22.5) / 2
]
|> resolve_with_sign(sd)
lat_precision = ((lat_pos - trunc(lat_pos)) / 0.25) |> trunc()
lon_precision = ((lon_pos - trunc(lon_pos)) / 0.25) |> trunc()

tdc = [List.first(td) / 5.625, List.last(td) / 11.25]
third_digit = precision_index_patch(lat_precision, lon_precision)

patch =
[index_patch(fdc), index_patch(sdc), index_patch(tdc)]
|> Enum.join("")

patch
[first_digit, second_digit, third_digit]
|> Enum.join("")
end

defp index_patch([f_i, s_i]) when f_i > 0.5 and f_i <= 1 and s_i < -0.5 and s_i >= -1, do: '0'
defp index_patch([f_i, s_i]) when f_i > 0.5 and f_i <= 1 and s_i < 0 and s_i >= -0.5, do: '1'
defp index_patch([f_i, s_i]) when f_i > 0.5 and f_i <= 1 and s_i < 0.5 and s_i >= 0, do: '2'
defp index_patch([f_i, s_i]) when f_i > 0.5 and f_i <= 1 and s_i < 1 and s_i >= 0.5, do: '3'

defp index_patch([f_i, s_i]) when f_i > 0 and f_i <= 0.5 and s_i < -0.5 and s_i >= -1, do: '4'
defp index_patch([f_i, s_i]) when f_i > 0 and f_i <= 0.5 and s_i < 0 and s_i >= -0.5, do: '5'
defp index_patch([f_i, s_i]) when f_i > 0 and f_i <= 0.5 and s_i < 0.5 and s_i >= 0, do: '6'
defp index_patch([f_i, s_i]) when f_i > 0 and f_i <= 0.5 and s_i < 1 and s_i >= 0.5, do: '7'

defp index_patch([f_i, s_i]) when f_i > -0.5 and f_i <= 0 and s_i < -0.5 and s_i >= -1, do: '8'
defp index_patch([f_i, s_i]) when f_i > -0.5 and f_i <= 0 and s_i < 0 and s_i >= -0.5, do: '9'
defp index_patch([f_i, s_i]) when f_i > -0.5 and f_i <= 0 and s_i < 0.5 and s_i >= 0, do: 'A'
defp index_patch([f_i, s_i]) when f_i > -0.5 and f_i <= 0 and s_i < 1 and s_i >= 0.5, do: 'B'

defp index_patch([f_i, s_i]) when f_i > -1 and f_i <= -0.5 and s_i < -0.5 and s_i >= -1, do: 'C'
defp index_patch([f_i, s_i]) when f_i > -1 and f_i <= -0.5 and s_i < 0 and s_i >= -0.5, do: 'D'
defp index_patch([f_i, s_i]) when f_i > -1 and f_i <= -0.5 and s_i < 0.5 and s_i >= 0, do: 'E'
defp index_patch([f_i, s_i]) when f_i > -1 and f_i <= -0.5 and s_i < 1 and s_i >= 0.5, do: 'F'

defp sign(number) when number < 0, do: -1
defp sign(number) when number >= 0, do: 1
defp main_index_patch(index) do
['8', '9', 'A', 'B', 'C', 'D', 'E', 'F', '0', '1', '2', '3', '4', '5', '6', '7']
|> Enum.at(index)
end

defp resolve_with_sign([first, second], [first2, second2]) do
defp precision_index_patch(index1, index2) do
[
do_resolve_with_sign(first, first2),
do_resolve_with_sign(second, second2)
['0', '1', '2', '3'],
['4', '5', '6', '7'],
['8', '9', 'A', 'B'],
['C', 'D', 'E', 'F']
]
end

defp do_resolve_with_sign(x1, x2) do
if sign(x1) == sign(x2) do
x1
else
x2 / 2
end
|> Enum.at(index1)
|> Enum.at(index2)
end
end
3 changes: 3 additions & 0 deletions test/archethic/bootstrap_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,9 @@ defmodule ArchEthic.BootstrapTest do
MockDB
|> stub(:get_first_public_key, fn _ -> first_public_key end)

MockGeoIP
|> stub(:get_coordinates, fn {200, 50, 20, 10} -> {0.0, 0.0} end)

assert :ok =
Bootstrap.run(
{200, 50, 20, 10},
Expand Down
20 changes: 15 additions & 5 deletions test/archethic/p2p/geo_patch_test.exs
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
defmodule ArchEthic.P2P.GeoPatchTest do
@moduledoc """
This module defines the test case to be used by
geopatch tests.
"""

use ExUnit.Case

alias ArchEthic.P2P.GeoPatch

import Mox

test "from_ip/1 should compute patch from coordinates" do
expect(MockGeoIP, :get_coordinates, fn ip ->
stub(MockGeoIP, :get_coordinates, fn ip ->
case ip do
# Spain (Alicante)
{88, 22, 30, 229} ->
Expand All @@ -23,12 +28,17 @@ defmodule ArchEthic.P2P.GeoPatchTest do
# Switzerland (Zurich)
{109, 164, 214, 168} ->
{47.366670, 8.550000}

# Edge value
{1, 2, 3, 4} ->
{-45.0, 0.0}
end
end)

assert "511" == GeoPatch.from_ip({88, 22, 30, 229})
assert "500" == GeoPatch.from_ip({161, 235, 112, 33})
assert "410" == GeoPatch.from_ip({15, 62, 246, 57})
assert "266" == GeoPatch.from_ip({109, 164, 214, 168})
assert "F1B" == GeoPatch.from_ip({88, 22, 30, 229})
assert "C1D" == GeoPatch.from_ip({161, 235, 112, 33})
assert "A1A" == GeoPatch.from_ip({15, 62, 246, 57})
assert "021" == GeoPatch.from_ip({109, 164, 214, 168})
assert "0E0" == GeoPatch.from_ip({1, 2, 3, 4})
end
end
2 changes: 2 additions & 0 deletions test/test_helper.exs
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,5 @@ Mox.defmock(MockGeoIP, for: ArchEthic.P2P.GeoPatch.GeoIP)
Mox.defmock(MockUCOPriceProvider, for: ArchEthic.OracleChain.Services.UCOPrice.Providers.Impl)

Mox.defmock(MockMetricsCollector, for: ArchEthic.Metrics.Collector)

Application.put_env(:archethic, ArchEthic.P2P.GeoPatch.GeoIP, MockGeoIP)