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

Add timezone grid caching to CLI (#17) #21

Merged
merged 4 commits into from
Aug 28, 2024

Conversation

borisnezlobin
Copy link
Contributor

I've gone ahead and implemented the changes described in Galen's comment. The only notable change in behaviour is that now, by default, find_sun and find will attempt to read from timezone_grid.json (even if no --grid argument is passed). If this is unwanted, I can change it back, I just thought it would be a good default.

While testing, I found that finder.generate_timezone_grid() generates a JSON file that doesn't match the timezone_grid.json currently in this repo... is that intentional? Also, shadowfinder generate_timezone_grid will overwrite the file it's given — should that be changed?

@GalenReich
Copy link
Collaborator

finder.generate_timezone_grid() generates a JSON file that doesn't match the timezone_grid.json currently in this repo

This isn't intentional, and is probably not desired either! The behaviour is controlled by these defaults:

self.angular_resolution = 0.5
self.min_lat = -60
self.max_lat = 85
self.min_lon = -180
self.max_lon = 180

It would be good to understand why this is happening

@borisnezlobin
Copy link
Contributor Author

I've just checked a lot more thoroughly (before, I was just looking at files changed, now I ran it through diffchecker.com) — the only difference is a trailing newline is removed. Sorry about that! I've also updated the tests so the reflect the new output.

Copy link
Collaborator

@GalenReich GalenReich left a comment

Choose a reason for hiding this comment

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

In general it looks good!

One small thing, find_sun needs updating to include the grid argument.

src/shadowfinder/cli.py Show resolved Hide resolved
@GalenReich
Copy link
Collaborator

I've just checked a lot more thoroughly (before, I was just looking at files changed, now I ran it through diffchecker.com) — the only difference is a trailing newline is removed. Sorry about that! I've also updated the tests so the reflect the new output.

No worries at all, better for it to be a newline than an entirely different set of points! So thank you for noticing the difference and double checking!

@borisnezlobin
Copy link
Contributor Author

One small thing, find_sun needs updating to include the grid argument.

...oops. I've fixed it now, I think. Would it be a good idea to add the modified timezone_grid.json to Git (and, I think, disable the pre-commit formatting for it)?

@borisnezlobin
Copy link
Contributor Author

bump. Anything else that needs to be fixed?

@GalenReich GalenReich self-requested a review August 28, 2024 10:12
@GalenReich
Copy link
Collaborator

This all looks good - apologies about the delay, and thanks for the bump! 🙌

@GalenReich GalenReich merged commit 578d5ac into bellingcat:main Aug 28, 2024
4 checks passed
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.

2 participants