Skip to content

Colocate tests w/ objects tested; switch from unittest to pytest#89

Merged
belsten merged 20 commits intorctn:mainfrom
dpaiton:colocate-tests
Jan 23, 2025
Merged

Colocate tests w/ objects tested; switch from unittest to pytest#89
belsten merged 20 commits intorctn:mainfrom
dpaiton:colocate-tests

Conversation

@dpaiton
Copy link
Collaborator

@dpaiton dpaiton commented Dec 31, 2024

Issues resolved

#83

Description

I made three structural changes

  • remove unittest dependency in favor of pytest.
    Pytest is a more flexible framework, and will allow us to extend the testing environment beyond what unittest allows.
  • break out priors & inference files into folders.
    This structure trades file complexity for directory complexity, which has a few upsides.
    • PRs are going to be easier to review if you think about files as containers holding code snippets. You can quickly see what was impacted by looking at the list of files changed.
    • testing is more clear (for reasons referenced in proposal: colocate tests with files being tested #83).
    • operating systems and IDEs are used to navigating directories, so there is no extra burden there. Directory views (such as the file explorer in VSCode) now has more information.
    • you avoid massive files that are hard to load, read, grep, etc.
  • move tests to be located by the files they are testing, and changed names to have universal *_test.py format.

Notes for reviewer

Reviewing commit-by-commit will probably be easier than the full diffs, since much of the changes were programatic.

The third from last commit updates tolerances on two tests. These tests would fail on main as well if you change the torch manual seed. We should consider setting up a reproducible-but-random sequence of seeds, aka "fuzz testing", for CI tests to give these inference methods more coverage.

run: pip install -r requirements.txt
run: |
python -m venv --upgrade-deps .venv
source .venv/bin/activate
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

using a venv here to emulate how we suggest people set up in the README.

- 'examples/**'
branches:
- main
pull_request:
Copy link
Collaborator Author

@dpaiton dpaiton Dec 31, 2024

Choose a reason for hiding this comment

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

This will now run the tests after the PR was merged, which is a good failsafe for the production branch (main)

2. Create a new branch to contain your changes.
2. `add`, `commit`, and `push` your changes to this branch.
3. Create a pull request (PR). See more information on submitting a PR request below.
1. Fork `main` branch.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cloning does not work because outside people do not have write permission. One has to fork to contribute, which is standard practice for open-source repos.

5. Submit your pull request and add reviewers.

1. If necessary, please **write your own unit tests** and place them near the code being tested. High-level tests, such as integration or example tests can be placed in the top-level "tests" folder.
2. Verify that all tests are passed by running `python -m pytest .`.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

using pytest instead of unittest


# data
raw_data
data
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added this because that is where the tutorial and example notebooks often look. We could also rename all of those hardcoded paths to "raw_data" and not add a "data" field here.

@dpaiton dpaiton marked this pull request as ready for review December 31, 2024 21:19
@belsten
Copy link
Collaborator

belsten commented Jan 15, 2025

I think this all looks great! Breaking the inference/prior code into individual files is a big change but I do think it is better for testing, and doesn't change the user's interface. That said, is there any reason sparsecoding/datasets, sparsecoding/models, sparsecoding/dictionaries , and sparsecoding/visualization shouldn't have the same treatment? I realize we don't have testing for this code yet, but I think thats all the more reason to format this code in the same manner (to motivate us to write tests).

I'm not so much asking you to do this, but more so if there is any reason not to do this.

@dpaiton
Copy link
Collaborator Author

dpaiton commented Jan 15, 2025

I think this all looks great! Breaking the inference/prior code into individual files is a big change but I do think it is better for testing, and doesn't change the user's interface. That said, is there any reason sparsecoding/datasets, sparsecoding/models, sparsecoding/dictionaries , and sparsecoding/visualization shouldn't have the same treatment? I realize we don't have testing for this code yet, but I think thats all the more reason to format this code in the same manner (to motivate us to write tests).

I'm not so much asking you to do this, but more so if there is any reason not to do this.

Yup, I think so. Best to have consistent formatting everywhere. I can follow-up with this later. I made issue #90 for now.

@belsten
Copy link
Collaborator

belsten commented Jan 23, 2025

I think this all looks great! Breaking the inference/prior code into individual files is a big change but I do think it is better for testing, and doesn't change the user's interface. That said, is there any reason sparsecoding/datasets, sparsecoding/models, sparsecoding/dictionaries , and sparsecoding/visualization shouldn't have the same treatment? I realize we don't have testing for this code yet, but I think thats all the more reason to format this code in the same manner (to motivate us to write tests).
I'm not so much asking you to do this, but more so if there is any reason not to do this.

Yup, I think so. Best to have consistent formatting everywhere. I can follow-up with this later. I made issue #90 for now.

Makes sense! Thanks for making the issue.

@belsten belsten merged commit 0d11026 into rctn:main Jan 23, 2025
2 checks passed
@dpaiton dpaiton deleted the colocate-tests branch February 7, 2025 22:08
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