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

CLI doesn't have a caching strategy #17

Open
GalenReich opened this issue Jun 21, 2024 · 3 comments
Open

CLI doesn't have a caching strategy #17

GalenReich opened this issue Jun 21, 2024 · 3 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@GalenReich
Copy link
Collaborator

At the moment the CLI recomputes the grid of timezones (the most expensive part of the computation) every time it is executed. Which makes the process very slow if a user calls shadowfinder multiple times.

Instead it would be good if a user could provide a path to a timezone json file.

It would also be good if there was a subcommand to generate the timezone file.

The end result would be something like:

shadowfinder generate_timezone_grid # creates timezone_grid.json in the working directory (or the given path?)
shadowfinder find 10 5 2024-02-29 13:59:59 --time_format=utc --grid=timezone_grid.json
@GalenReich GalenReich added enhancement New feature or request good first issue Good for newcomers labels Jun 21, 2024
@borisnezlobin
Copy link
Contributor

I'm new here, but I can work on this. Does this entail just adding a CLI command to do essentially what is in the README (and already written in Python, just not the CLI)?

@GalenReich
Copy link
Collaborator Author

There are a few parts to this:

  • Add a new CLI subcommand called generate_timezone_grid that calls finder.generate_timezone_grid() and finder.save_timezone_grid() as in the Notebook implementation
  • Modify the existing find CLI subcommand to take an optional argument grid that loads the generated file before running the search
  • Modify the quick_find function (or refactor it away)
    def quick_find(self):
    self.generate_timezone_grid()
    self.find_shadows()
    fig = self.plot_shadows()
    to avoid generating the grid if it has been loaded by the new grid argument

Hope that helps! Let me know if you run into any issues when getting set up.

@borisnezlobin
Copy link
Contributor

Thanks Galen! I've done these things (correctly, I hope) and made a PR (#21). Could you look over it and let me know if it needs changes?

GalenReich added a commit that referenced this issue Aug 28, 2024
* add cli caching, will read from timezone.json or --grid argument

* update output in test_executable_without_args

* update find_sun to use cached timezone_grid.json

* Bump minor version number

---------

Co-authored-by: Galen Reich <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants