Skip to content

Updates to the package during JOSS Review#4

Open
djpasseyjr wants to merge 21 commits intomainfrom
JOSS-Review
Open

Updates to the package during JOSS Review#4
djpasseyjr wants to merge 21 commits intomainfrom
JOSS-Review

Conversation

@djpasseyjr
Copy link
Owner

@djpasseyjr djpasseyjr commented Jan 26, 2026

See #3

@codecov
Copy link

codecov bot commented Jan 26, 2026

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

Copy link

@Jacob-Stevens-Haas Jacob-Stevens-Haas left a comment

Choose a reason for hiding this comment

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

A few quick suggestions where my comments overlapped with the other reviewer.

Comment on lines +37 to +38
"surd @ git+https://github.com/djpasseyjr/surd.git",
"pyclustering @ git+https://github.com/djpasseyjr/pyclustering",
Copy link

@Jacob-Stevens-Haas Jacob-Stevens-Haas Feb 1, 2026

Choose a reason for hiding this comment

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

I don't think this level of sophistication is a requirement for JOSS - just suggestion from someone else who's followed the pattern you suggested and run into difficulties keeping dependencies in sync when packages co-evolved.

Suggested change
"surd @ git+https://github.com/djpasseyjr/surd.git",
"pyclustering @ git+https://github.com/djpasseyjr/pyclustering",
"surd @ git+https://github.com/djpasseyjr/surd.git@0b4fb1c",
"pyclustering @ git+https://github.com/djpasseyjr/pyclustering.git@6bb2311",

What you're essentially doing is vendoring forks of the original surd/pyclustering. This can be tricky to do correctly. If you're expecting to potentially reuse your forks in other projects (and potentially change them/keep them up to date with their upstream), it's helpful to pin dependencies here to a specific commit. If your development plan for interfere and the dependency forks has all evolving in tandem, it's better to vendor them as git submodules (and configured in pyproject.toml so pip install interfere also installs them), so that changes to interefere are always tested against the correct vendored version. There's other ways of handling vendoring, e.g. the vendoring package, but I haven't used them.

fail-fast: false
matrix:
python-version: ["3.9", "3.10", "3.11"]
python-version: ["3.9", "3.10", "3.11", "3.12", "3.13"]

Choose a reason for hiding this comment

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

Could you also specify a minimum python version in pyproject.toml?

README.md Outdated
- `pysindy`
- `scikit-learn`
- `statsmodels`
- `typing_extensions`

Choose a reason for hiding this comment

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

NIT: listing the dependencies on the readme is not required and I think clutters the page. If anything, just list the extra dependency groups available (dev, methods) and what they do - not their contents.

@djpasseyjr
Copy link
Owner Author

djpasseyjr commented Feb 2, 2026

#5

Adding linter and formatting support.

@djpasseyjr
Copy link
Owner Author

#5 #6

I'm addressing the time it takes to run the full test suite by instructing contributors to just test local changes and let the full suite run in the GitHub action on PR. I also detail how to test individual dynamic models or methods

@djpasseyjr
Copy link
Owner Author

#7

@djpasseyjr
Copy link
Owner Author

#8 #7 The two commits above address these issues.

@djpasseyjr
Copy link
Owner Author

#10 The commits above add a benchmark documentation page to address the issue.

djpasseyjr and others added 10 commits February 25, 2026 08:59
Clarified the description of hyperparameter optimization in forecasting methods and added context regarding observational data and interventions.
Clarified figure caption regarding the simulation of the quadratic Belozyorov system and the representation of stochastic noise.
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