Skip to content

2669 improve location accuracy for street newsflashes#2708

Open
tkalir wants to merge 9 commits intodata-for-change:devfrom
tkalir:2669-Improve-location-accuracy-for-street-newsflashes
Open

2669 improve location accuracy for street newsflashes#2708
tkalir wants to merge 9 commits intodata-for-change:devfrom
tkalir:2669-Improve-location-accuracy-for-street-newsflashes

Conversation

@tkalir
Copy link
Copy Markdown
Collaborator

@tkalir tkalir commented Oct 3, 2024

No description provided.

@atalyaalon
Copy link
Copy Markdown
Collaborator

@tkalir no urgency here, whenever you have time - can you please check and fix Pylint, Black and failing tests?

Comment thread requirements.txt
openai==1.45.0
langchain==0.2.16
langchain_openai==0.1.25
python-dotenv No newline at end of file
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is python-dotenv necessary?
Also, SQLAlchemy==1.4 modification necessary?
(I think we should perform packages upgrade, but it will be in a different pr with suitable tests)

gmaps = googlemaps.Client(key=secrets.get("GOOGLE_MAPS_KEY"))
geocode_result = gmaps.reverse_geocode((latitude, longitude))

print(geocode_result)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you remove all prints in this code?

Copy link
Copy Markdown
Collaborator

@atalyaalon atalyaalon left a comment

Choose a reason for hiding this comment

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

@tkalir All in all looks good., added some small comments
In addition, in the case that location is on a "boarder" of two municipalities and nearest accidents might be in different "yishuv_name", this case is not handled.
I think this should be taken into account in the current solution which might modify it a bit.

@atalyaalon
Copy link
Copy Markdown
Collaborator

atalyaalon commented Oct 15, 2024

@ziv17 can you review this one as well? (No urgency here)
I want to make sure I didn't miss anything in edge cases

@atalyaalon
Copy link
Copy Markdown
Collaborator

atalyaalon commented Oct 15, 2024

@tkalir I added key to secrets.
Note that github-secrets are not accessible for PRs from forks, therefore tests in this PR will fail, but in merging dev to master they will not.
However, we do want to enable tests running from forks, henece I suggest wrap the secret in a function, to avoid tests failures in tests from forks

Comment thread anyway/llm.py
from enum import Enum
from anyway import secrets

api_key = secrets.get("OPENAI_API_KEY")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suggest inserting the secret into a function, to avoid tests failures in tests from forks

Comment thread anyway/llm.py
model = ChatOpenAI(api_key=api_key, temperature=0)


def match_streets_with_langchain(street_names, location):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi, I prefer to have type hints. It makes the reading much easier.

Comment thread anyway/llm.py
model = ChatOpenAI(api_key=api_key, temperature=0)


def match_streets_with_langchain(street_names, location):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this code used?

Copy link
Copy Markdown
Collaborator

@ziv17 ziv17 left a comment

Choose a reason for hiding this comment

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

Hi @tkalir , Very nice!
I learned a lot 😊

  1. I assume this PR is only the beginning, and probably it is possible to extract more information, like the yishuv, resolution, etc.
  2. Is it possible to get the level of confidence the AI tool is has in its answer? In certain cases it may be more accurate than the lat / lon that we have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Improve location accuracy for newsflashes with Street resolution

3 participants