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

Complete 2nd rebuttal, fix #402 #404

Merged
merged 3 commits into from
Mar 4, 2024
Merged

Conversation

gzagatti
Copy link
Contributor

@gzagatti gzagatti commented Mar 4, 2024

This PR is the second round of rebuttal. It fixes #402.

Please also see my responses to the reviewer in the original issue.

This time I have made all my changes using the \comment macro to make it easier to spot all my changes in magenta in the compiled PDF. Once we are satisfied with all the changes. I will remove the macro and leave only the text. I hope that it makes it easier to review it that way.

cc: @gdalle

@gzagatti
Copy link
Contributor Author

gzagatti commented Mar 4, 2024

Find the compiled paper here for easier reference.

@isaacsas
Copy link
Member

isaacsas commented Mar 4, 2024

LGTM. Should I merge?

One minor comment, if you wanted to add a sentence speculating on why NRM/Coevolve don't offer the top performance on constant rate problems, you could say that it is due to the cost of updating their underlying indexed priority queue data structure which stores the next event times. A table-based data-structure would be expected to be more competitive this is the Sanft-Othmer paper, and I suspect a real cache-optimized indexed priority queue would give very good performance (but perhaps not beating RSSACR on the largest problems). The latter would be a great project actually that if as performant as I expect could be a J. Chem. Phys. paper...

@gzagatti
Copy link
Contributor Author

gzagatti commented Mar 4, 2024

Thanks for the quick turnaround! Let me add your minor comment and remove the comment markup before you merge.

@gzagatti
Copy link
Contributor Author

gzagatti commented Mar 4, 2024

Draft with minor comment added.

I will now remove the comment markup and let you know once it is good to merge.

@isaacsas
Copy link
Member

isaacsas commented Mar 4, 2024

The highlighting makes it much easier to quickly see what has changed!

@gzagatti
Copy link
Contributor Author

gzagatti commented Mar 4, 2024

Now, it's good to merge.

@isaacsas isaacsas merged commit 698d296 into SciML:juliacon2023 Mar 4, 2024
2 of 3 checks 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