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

Paper: RoughPy #904

Merged
merged 32 commits into from
Sep 25, 2024
Merged

Paper: RoughPy #904

merged 32 commits into from
Sep 25, 2024

Conversation

inakleinbottle
Copy link
Contributor

@inakleinbottle inakleinbottle commented May 21, 2024

If you are creating this PR in order to submit a draft of your paper, please name your PR with Paper: <title>. An editor will then add a paper label and GitHub Actions will be run to check and build your paper.

See the project readme for more information.

Editor: Charles Lindsey @cdlindsey

Reviewers:

@cbcunc cbcunc added the paper This indicates that the PR in question is a paper label May 21, 2024
Copy link

github-actions bot commented May 21, 2024

Curvenote Preview

Directory Preview Checks Updated (UTC)
papers/sam_morley 🔍 Inspect 68 checks passed (7 optional) Aug 3, 2024, 8:45 PM

Copy link
Collaborator

@PaulJWright PaulJWright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Firstly I would like to apologise to the authors as the subject of this paper is outwith my wheelhouse, and as such have provided minor comments where I believe this will help the manuscript.

Review:

This manuscript describes RoughPy, a Python package for viewing streams of data through the lens of rough paths and using the tools and techniques of signatures. The manuscript is well-written and clear, with good accompanying documentation at https://roughpy.org/.

The codebase at https://github.com/datasig-ac-uk/RoughPy seems complete with well-written documentation, testing (including automated testing through Github Actions), and regular releases. There are currently no outstanding issues (https://github.com/datasig-ac-uk/RoughPy/issues) that cause concern, however, it would be nice to merge in datasig-ac-uk/RoughPy#91 or an equivalent. The inclusion of an issue template is noted and appreciated.

I am personally unsure of the reasoning behind the tests/build (windows-2019) failure in the CI:

FAILED tests/streams/test_lie_increment_path.py::test_from_array_width_3_2_increments - AssertionError: { 1() 3/3(1) 7(2) 1(3) } != { 1() 3(1) 7(2) 1(3) }

while ubuntu-latest/macos-11 pass, but would encourage the authors to investigate a fix to this issue.

papers/sam_morley/roughpy.md Show resolved Hide resolved
papers/sam_morley/roughpy.md Outdated Show resolved Hide resolved
papers/sam_morley/roughpy.md Outdated Show resolved Hide resolved
@cdlindsey
Copy link
Contributor

cdlindsey commented Jul 26, 2024

Very interesting paper and package. Could you provide a link to more detailed examples if they are too complex to fully fit into the text?

Copy link
Collaborator

@cliffckerr cliffckerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a mathematician so I confess most of this paper went over my head, but from what I could tell it was well written. For me it would be helpful if there were more of a link between the very high-level applications (e.g. detecting interference) and the very simple example (computing the signature of the word 'scipy'). I know the authors point to the documentation for more information, but perhaps a slightly longer example would be helpful. However, I recognize I am probably not the intended audience for this paper, so it's fine to pass on this suggestion.

Minor comments:

  • Typo, 'On simple'
  • What does it mean "speed at which events occur"? Are we talking about time series, text streams, event time stamps, etc.?
  • Typo, 'need to be done' (no period)
  • What is 'the sequel'?
  • Typo, 'unversal'
  • Typo, 'In order to property'

@inakleinbottle
Copy link
Contributor Author

Thanks so much for your comments. I'm looking to fix the issues that you highlighted.

@cdlindsey I cited the Datasig website for the more complex examples, but I guess this wasn't sufficiently clear. I will adjust my wording to make this more clear.

@cliffckerr Part of the purpose of RoughPy is that non-mathematicians can make use of the complicated machinery that exists below the surface, so in some way you are exactly part of the target audience for the package. Unfortunately, rough analysis is not sufficiently well-known outside of the mathematics community that I can reference it actually defining everything. We do have a bit of space to play with in this paper, so I can certainly try to explain the connection between the example and the more involved applications. "In the sequel" is used to mean "the rest of the paper". I'm now realizing that it might not be so ubiquitous.

@inakleinbottle
Copy link
Contributor Author

OK, I believe I have addressed all the issues raised by the reviewers, and a few additional things I noticed too. This should be my final version.

@cbcunc cbcunc merged commit 271dd59 into scipy-conference:2024 Sep 25, 2024
4 checks passed
@inakleinbottle inakleinbottle deleted the sam_morley branch October 7, 2024 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
paper This indicates that the PR in question is a paper ready-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants