-
Notifications
You must be signed in to change notification settings - Fork 4
implement tensors in binsparse #21
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
implement tensors in binsparse #21
Conversation
TODO: you need to apparently set the format key.
@BenBrock Here's the PR to add tensors to the spec, please review when you get the chance! |
@junikimm717 @willow-ahrens Thanks for submitting the PR! At first glance everything looks generally good. I see two small problems:
I would also prefer to add a new flag I'm also happy to fix these issues myself, but I have a busy week with a deadline coming up, so I probably won't be able to spend time on this until next week. |
Currently, the tensor tests only run if Julia is installed. Is this sufficient? |
Yes, requiring Julia for the tensor tests is fine for now, as long as we can run the CI in GitHub actions. (That should be no problem.) |
`-DCMAKE_EXPORT_COMPILE_COMMANDS=ON` to generate automatically.)
.github/workflows/ci.yml
Outdated
- name: Install Julia | ||
run: curl -fsSL https://install.julialang.org | sh -s -- -y | ||
- name: Disable Julia precompilation | ||
run: julia -e 'using PrecompileTools, Preferences; set_preferences!(PrecompileTools, "precompile_workloads" => false; force=true)' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@willow-ahrens Does turning off precompilation in Julia like this make sense?
I'm doing this because precompilation takes a while, and it seems to me that the CI job ought to be faster overall without precompilation if we're installing and using the package once. (Julia will still JIT compute intensive portions, right?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the builds, it doesn't appear to actually turn off precompilation, and I'm not sure why. Let me know if you have any thoughts.
bsp_tensor_t
field is returned.