Skip to content

Cut off nose#1141

Merged
dwhswenson merged 19 commits into
openpathsampling:masterfrom
dwhswenson:spite-face
Apr 26, 2024
Merged

Cut off nose#1141
dwhswenson merged 19 commits into
openpathsampling:masterfrom
dwhswenson:spite-face

Conversation

@dwhswenson

@dwhswenson dwhswenson commented Jan 14, 2024

Copy link
Copy Markdown
Member

Resolves #1140, and will allow us to enable Python 3.12 testing (at least in minimal testing; unsure whether some upstreams are caught up yet).

This is a decent bit of tedious work (which is why it hasn't been done yet.) I can facilitate it with some vim macros, but there will be edge cases, so results from macros need to be manually validated. My guess is that I'll do it in the following steps:

  • Replace assert_equal(a, b) with assert a == b
  • Replace assert_not_equal(a, b) with assert a != b
  • Replace raise SkipTest() with pytest.skip()
  • Replace @raises(...) with with pytest.raises(...)
  • Replace assert_almost_equal, probably with things from numpy.testing
  • Replace various methods in test_helpers that are built on nose
  • Clean up any remaining bits that aren't included above
  • Remove nose from the build process

Adding Python 3.12 to the CI matrix will come in a later PR.

@dwhswenson

Copy link
Copy Markdown
Member Author

This is ready for review and comment. Without review, I will merge this after at least 24 hours, no sooner than Fri 26 Apr 02:00 GMT (Thu 25 Apr 21:00 my local).

This is a huge PR in terms of changes, but all of them are individually small and repetitive. I converted legacy nose to pytest with the help of a number of vim macros. But do at least take a look at the many nose-based puns I've been making in commit messages, as well as in the PR title/branch name. Otherwise, like most of my jokes, these will be spoken into the void, only for my own amusement.

@dwhswenson dwhswenson requested a review from sroet April 25, 2024 01:35
@dwhswenson dwhswenson changed the title [WIP] Cut off nose Cut off nose Apr 25, 2024
@sroet

sroet commented Apr 25, 2024

Copy link
Copy Markdown
Member

But do at least take a look at the many nose-based puns I've been making in commit messages, as well as in the PR title/branch name. Otherwise, like most of my jokes, these will be spoken into the void, only for my own amusement.

Thanks for the heads up, I do appreciate them 😀 Will walk through the PR and see how far I get

@sroet sroet left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a huge PR in terms of changes, but all of them are individually small and repetitive.

This makes it pretty easy to review, and unfortunately also means my many comments are also pretty repetative

To summarize:
Couple question about a test being skipped after it has already completed
Couple style nitpicks
Couple comments about unnecessary backslashes
Couple comments about me preferring no backslashes (feel free to ignore these)
One test case went missing (this is the main one that needs to be changed)

Other things I noticed:

  • you sometimes go for assert x and sometimes for assert x is True from assert_equal(x, True), either one is fine by me (while I think assert x is technically the better replacement)

Other things I checked:

  • I did run a grep -R "nose" ./ on this branch and it only matched a .git object

LGTM otherwise

Comment thread openpathsampling/tests/test_bias_function.py Outdated
Comment thread openpathsampling/tests/test_channel_analysis.py Outdated
Comment thread openpathsampling/tests/test_ensemble.py
Comment thread openpathsampling/tests/test_ensemble.py Outdated
Comment thread openpathsampling/tests/test_ensemble.py Outdated
Comment thread openpathsampling/tests/test_trajectory_transition_analysis.py Outdated
Comment thread openpathsampling/tests/test_transitions.py Outdated
Comment thread openpathsampling/tests/test_transitions.py Outdated
Comment thread openpathsampling/tests/test_transitions.py Outdated
Comment thread openpathsampling/tests/test_transitions.py Outdated
Co-authored-by: Sander Roet <sanderroet@hotmail.com>

@dwhswenson dwhswenson left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

First pass reply; and accepting most (if not all?) suggestions. Then I'll do a second pass to address the rest.

Comment thread openpathsampling/tests/test_ensemble.py
Comment thread openpathsampling/tests/test_ensemble.py Outdated
Comment thread openpathsampling/tests/test_helpers.py
Comment thread openpathsampling/tests/test_movescheme.py Outdated
dwhswenson and others added 2 commits April 25, 2024 17:46
Note to self: commit batch BEFORE finishing review. Otherwise the batch resets? Why, GitHub? Why?

Co-authored-by: Sander Roet <sanderroet@hotmail.com>

@dwhswenson dwhswenson left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Now for one more pass through to see what points remain unresolved....

Comment thread openpathsampling/tests/test_ensemble.py
Comment thread openpathsampling/tests/test_storage.py
@dwhswenson

Copy link
Copy Markdown
Member Author

Changes should be all addressed! Take a look when you can, @sroet, and mark it green if ready!

Replies to a couple other points:

you sometimes go for assert x and sometimes for assert x is True from assert_equal(x, True), either one is fine by me (while I think assert x is technically the better replacement)

Yeah, I agree. First macro I added was (of course) assert_equal(X, Y) to assert X == Y. Eventually I saw enough of the assert_equal(X, True) that I made a macro to make it into assert X (and assert not X for False). Which one you see depends on how far into the process I was when I hit that file.

I did run a grep -R "nose" ./ on this branch and it only matched a .git object

git ls-files | xargs grep "nose" 😉 I have far too many random untracked files in my OPS directory.... (But that's how I found the random mention in an ipynb).

@sroet sroet left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! thanks for handling the comments (and the useful note in the batch commit)

@dwhswenson dwhswenson merged commit 85e3063 into openpathsampling:master Apr 26, 2024
@dwhswenson dwhswenson deleted the spite-face branch April 26, 2024 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python 3.12 breaks nose; all nose based testing should be removed

2 participants