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

JOSS Review Suggestions #4

Merged
merged 8 commits into from
Jul 27, 2024
Merged

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Jun 21, 2024

See #3

This PR attempts to resolve some of the suggestions from my review, along with a handful of typo fixes and similar edits.

Please feel free to accept or reject any of the changes in this PR. Some changes to benchmarks and Dockerfiles are slightly opinionated. If you want to reject any changes, you can just comment on the PR where you want to keep the current code, and revert the commits where those changes were made. (e.g. git revert 7a9840b) That will make it easier to track.


  • Fix typos in README.
  • Fix typos in example usage.
  • Use faster native Python benchmark.
  • Try simplifying Dockerfile for faster installation.
  • Speed up NumPy code by about a third.
  • Small paper edits.
  • Use nimble install --depsOnly.
  • Fix typos in test suite

@RMeli
Copy link
Contributor

RMeli commented Jul 16, 2024

Hi @amkrajewski, this PR looks very helpful to get openjournals/joss-reviews#6731 moving forward. Is there anything blocking its review and merge?

@amkrajewski
Copy link
Owner

@RMeli Thanks for the ping! I was waiting for Reviewer 3 before making any modifications (which I got last week). I am traveling right now, but I should be able to start working on revisions soon and finish them next week.

@amkrajewski amkrajewski self-requested a review July 27, 2024 16:53
@amkrajewski
Copy link
Owner

Thank you, @bdice, for your thorough and impactful contributions in this PR, with attention to detail and performance optimizations. I very much appreciate these!

I will now re-benchmark everything, adjust the table in the paper, and get back to other points you've raised in #3

@amkrajewski amkrajewski merged commit a43699d into amkrajewski:main Jul 27, 2024
@bdice
Copy link
Contributor Author

bdice commented Jul 27, 2024

Glad it helped!

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.

3 participants