Fix GeoPatch algorithm #264

Neylix · 2022-03-24T00:50:02Z

Description

Fixes bug on GeoPatch algorithm to match the following rules :

first digit represent latitude patch
second digit represent longitude patch
third digit is a precision within the previous two digits

Fixes #253

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Runned a script with all coordinates possibilities (2 nested loop from -90 to 90 and -180 to 180), result is 2048 distinct geo patch (16x8x16)
Geo patch match the grid coordinates shown in issue (pos 0,0 is patch 000, pos -1,-1 is patch FFF)

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

Fixes archethic-foundation#253

Neylix · 2022-03-24T00:52:36Z

Requested review form @samuel-uniris @I-Archer-Zero

imnik11

Overall looks good and precision is improved

imnik11 · 2022-03-29T22:31:08Z

lib/archethic/p2p/geo_patch.ex

-    lat_sign = sign(lat)
-    lon_sign = sign(lon)
+    lat_pos = (lat + 90) / 11.25
+    lon_pos = (lon + 180) / 22.5


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

lat_pos

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

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

@I-Archer-Zero

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

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.

Hello !
I fixed calculation in lastest commits

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.

imnik11

@Neylix @internet-zero

The Edge case problem is till in the new Algo but I avoided it by taking that also in the range
so, earlier
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'

and when we input (-45, x) it crashed and it shall be avoided
by using just >= while condition check

Now
defp precision_patch(lat, lon) when lat >= 0 and lat <= 0.25 and lon >= 0 and lon <= 0.25, do: '0'

Neylix · 2022-04-03T20:30:41Z

@imnik11
Hello ! I did some tests with the values -45,x but it seems to work, I added these in test files and it going well.
Can you show the error you got ?

imnik11 · 2022-04-04T01:41:35Z

lib/archethic/p2p/geo_patch.ex

-      |> resolve_with_sign([lat, lon])
+    lon_pos = (lon + 180) / 22.5
+    # Adding 4 to have second digit hex value from C to 3
+    lat_pos = (lat + 90) / 22.5 + 4


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

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 ?

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

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.

ghost · 2022-04-08T08:20:14Z

Seems more clear now.
Could you add more tests to be sure close locations, would be closed about the geo-patch, for instance in the 1st and 2nd digit but maybe not the 3rd

Neylix · 2022-04-08T09:38:50Z

Added some tests with near location cities

ghost · 2022-04-08T10:03:21Z

test/archethic/p2p/geo_patch_test.exs

@@ -40,5 +56,9 @@ defmodule ArchEthic.P2P.GeoPatchTest do
    assert "A1A" == GeoPatch.from_ip({15, 62, 246, 57})
    assert "021" == GeoPatch.from_ip({109, 164, 214, 168})
    assert "8C0" == GeoPatch.from_ip({1, 2, 3, 4})
+    assert "F1F" == GeoPatch.from_ip({1, 1, 1, 1})


Is this distance good ? I mean it's almost close

Yes, first digit F means that longitude is >= -22.5 and < 0, when going in positive we return to 0. i.e 0 is >= 0 and < 22.5.
So just after FFF we have 000.

Ok .
@imnik11 can you have a last check ?

* Update geo patch calculation and correct out of bound index * Improve tests to check near location city * Fixes #253

Fix GeoPatch algorithm

e93bf68

Fixes archethic-foundation#253

ghost added external-contribution bug Something isn't working P2P Involve P2P networking labels Mar 24, 2022

imnik11 reviewed Mar 29, 2022

View reviewed changes

imnik11 reviewed Mar 30, 2022

View reviewed changes

Neylix added 2 commits April 3, 2022 22:00

Update geo patch calculation

488f860

update geo patch tests

094d088

Neylix requested review from internet-zero and imnik11 April 3, 2022 20:31

imnik11 reviewed Apr 4, 2022

View reviewed changes

Neylix added 2 commits April 4, 2022 12:24

Switch env value to config

9896535

Correct out of bound index

e5b0d04

Neylix requested a review from a user April 7, 2022 10:27

Convert list to tuple to improve execution time

7e67cde

Add near location city in test

6b722f9

ghost reviewed Apr 8, 2022

View reviewed changes

ghost merged commit aedde02 into archethic-foundation:develop Apr 19, 2022

Neylix deleted the Fix-GeoPatch-algorithm branch April 19, 2022 08:50

ghost mentioned this pull request Apr 27, 2022

improve mining error handling #289

Merged

ghost pushed a commit that referenced this pull request May 6, 2022

Fix GeoPatch algorithm (#264)

bc28401

* Update geo patch calculation and correct out of bound index * Improve tests to check near location city * Fixes #253

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix GeoPatch algorithm #264

Fix GeoPatch algorithm #264

Neylix commented Mar 24, 2022 •

edited

Loading

Neylix commented Mar 24, 2022

imnik11 left a comment

imnik11 Mar 29, 2022

imnik11 Mar 30, 2022

Neylix Mar 30, 2022 •

edited

Loading

ghost Mar 30, 2022

internet-zero Mar 30, 2022 •

edited

Loading

Neylix Mar 30, 2022

Neylix Apr 3, 2022

imnik11 Apr 4, 2022

imnik11 left a comment

Neylix commented Apr 3, 2022

imnik11 Apr 4, 2022

Neylix Apr 5, 2022

ghost Apr 7, 2022 •

edited by ghost

Loading

Neylix Apr 7, 2022

ghost commented Apr 8, 2022

Neylix commented Apr 8, 2022

ghost Apr 8, 2022

Neylix Apr 8, 2022

ghost Apr 8, 2022

Fix GeoPatch algorithm #264

Fix GeoPatch algorithm #264

Conversation

Neylix commented Mar 24, 2022 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

Neylix commented Mar 24, 2022

imnik11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neylix Mar 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

internet-zero Mar 30, 2022 • edited Loading

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

imnik11 left a comment

Choose a reason for hiding this comment

Neylix commented Apr 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Apr 7, 2022 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 8, 2022

Neylix commented Apr 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neylix commented Mar 24, 2022 •

edited

Loading

Neylix Mar 30, 2022 •

edited

Loading

internet-zero Mar 30, 2022 •

edited

Loading

ghost Apr 7, 2022 •

edited by ghost

Loading