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

Updates syngine client #280

Merged
merged 3 commits into from
Nov 21, 2024
Merged

Conversation

rmodrak
Copy link
Member

@rmodrak rmodrak commented Nov 15, 2024

Includes the following changes to how syngine Green's functions are cached locally

  • adds new cache_path option to syngine.Client()
  • if cache_path is not given, then attempts to download to MTUQ install directory (mtuq/data/greens_tensor/syngine/cache)
  • if MTUQ install directory is not writeable, then attempts to download to current working directory (./syngine/cache)
  • removes SYNGINE_CAHCE environment variable

@rmodrak
Copy link
Member Author

rmodrak commented Nov 15, 2024

Feel free to post any comments/reviews. Planning to merge on Tue Nov 19

@thurinj
Copy link
Member

thurinj commented Nov 18, 2024

Hi @rmodrak,

Just wondering if removing the SYNGINE_CACHE variable is a good idea. I have the impression that it makes things easier when working on the containerized version of the code.
Not completely the greatest priority, as I suspect it is doomed to be deprecated at some times in the future, but using the syngine cache path variable was required to make my introduction to mtuq Google Colab notebook work.

@rmodrak
Copy link
Member Author

rmodrak commented Nov 18, 2024

Hi @thurinj, Good point, we could delay merging to allow for discussion during one of the monthly calls. Ultimately, there is a requirement to pass a particular code scan that may force a handful of changes like this. Sorry for the disruption.

@rmodrak
Copy link
Member Author

rmodrak commented Nov 18, 2024

In the meantime, it might be worth double checking the new implementation

  • instead of an environment variable, it is now possible to specify the the Green's function cache path via an input argument to any of the syngine functions
  • if the cache path is not specified or incorrectly specified, it attempts to fall back to a writeable directory

My hope is that with the above fallback, old notebooks would still continue to work

@thurinj
Copy link
Member

thurinj commented Nov 20, 2024

Thanks for the explanation. If I can directly specify a path, I should be able to get pre-bundled syngine green's functions (I download a zip instead of waiting for the syngine fetching in my Google colab notebook) and then point to it.

@rmodrak rmodrak merged commit b202347 into uafgeotools:master Nov 21, 2024
1 check 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