P/fix kwave if #21
P/fix kwave if #21
Conversation
There was a problem hiding this comment.
Nice, thanks!
Commits are separated out nicely with each commit doing one thing, that's great
To start following our new guidelines in CONTRIBUTING.md:
- Every commit should reference the issue number with which it is associated.
can you make sure that issue numbers are referenced in the title and or body of each commit? It looks like two of the three commits are associated with #9. For the commit about adding the ability to disable gpu, you would have to open a new issue with a description of the thing being done (super short description is fine -- we just want to track which changes are for which requirement) and then reference that new issue in the commit.
For editing commit messages I usually do this by an interactive rebase -- let me know on discord or wherever if you could use help with that
notebooks/test_first.ipynb
Outdated
| "execution_count": 12, | ||
| "metadata": {}, | ||
| "outputs": [], | ||
| "outputs": [ |
There was a problem hiding this comment.
Did you mean to include cell outputs in this commit? It will be better to clear all cell output to avoid bloating the repo.
(This will stop being an issue once we do #15 😄 -- I can prioritize that next if interested)
There was a problem hiding this comment.
I did an interactive rebase and chose to reword the commits, but now I'm seeing a second set of them tracked in this PR... guessing I didn't fix that correctly
There was a problem hiding this comment.
Interactive rebase will change the commit hashes, so that's correct actually!
You would have to force-push the new commits to replace the previous commit history:
git push --force-with-lease
Regarding removing cell outputs: adding an additional commit that removes the cell output still leaves the output in the commit history, which continues to bloat the repo. What's needed ultimately is to change that commit that introduces the output and remove the output from it. Alternatively, the two commits (the one that introduces cell output and the one that removes it) can be squashed to achieve that same effect. Happy to hop onto a call if I can help in any way!
There was a problem hiding this comment.
(It looks like you did force-push so that's good.)
(But I see a confusing merge commit so maybe something went wrong?)
peterhollender
force-pushed
the
p/fix_kwave_if
branch
from
70d0f85 to
64814b2
Compare
ebrahimebrahim
left a comment
ebrahimebrahim
left a comment
There was a problem hiding this comment.
It looks good! Thanks for your patience with the new software processes :)
I am adding "also closes #22" to the PR description. Feel free to merge
ebrahimebrahim
force-pushed
the
p/fix_kwave_if
branch
from
64814b2 to
ecd4b73
Compare
Closes #9 by removing the gridweights interface altogether. I don't think that the implementation on my fork is going to be how a first-party fix gets completed, so it's not really worth keeping, and the performance on
masterseems to have improved at least somewhat, perhaps from another commit.Also incidentally fixes an issue where non-GPU based simulation wasn't getting called, which closes #22