Repository navigation
docs: tell contributors what CI runs and which documented commands do not work - #55
Merged
Merged
Conversation
The Running things section documented two commands that do not do what they claim. python profiling/visualize.py exits 0 and writes nothing, because the module has no entry point and never imports argparse. And make setup, test and lint fail on Windows, because the Makefile hardcodes a POSIX virtualenv layout. A contributor following either would believe the work had happened. Replaces them with the commands CI runs, which work on every platform, and states plainly what each broken path does instead. Adds a Continuous integration section naming the three checks, recording that ruff is pinned in two places that must be bumped together, and warning that ruff format also rewrites fenced python blocks in Markdown so a README edit alone can fail the lint job. Also records what a green suite does not mean. No CI job reads profiling/results/ and none can validate a performance claim, since hosted runners have no GPU. Correctness passing says the reference implementations agree with each other, not that the published numbers are right. That gap is issue 27. Adds a Style note that the state-space matrices keep their capital letters, since N803 and N806 are disabled deliberately.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
CONTRIBUTING.mdtold contributors to run two commands that do not do what they claim.python profiling/visualize.py <results.json>exits 0 and writes nothing. The module has noif __name__ == "__main__"block and never imports argparse, so it cannot be run as a script at all. That is #48, and it is why the A100 result set still has no plots or summary.make setup,make testandmake lintfail on Windows, because the Makefile hardcodes$(VENV)/bin. That is #36.A silent no-op is worse than a crash. Someone following the first command walks away believing the plots now exist. Both were documented as normal usage, with no caveat, in the file a new contributor reads first.
Separately, CI landed in #54 and nothing told contributors it exists, what it runs, or what can fail it.
What changed
Running things now leads with the four commands CI actually runs, which work on every platform:
makeis still mentioned, with the reason it does not work everywhere and a pointer to #36. The two broken entry points are stated plainly rather than removed, so a contributor who has already tried them understands what happened.New section, Continuous integration. Names the three checks. Records that ruff 0.17.0 is pinned in two places,
pyproject.tomlandci.yml, which must be bumped together. Warns thatruff formatalso rewrites fencedpythonblocks in Markdown, so a README edit with no Python in it can fail the lint job. That surprised me while writing #52 and it will surprise the next person too.What green does not mean. No CI job reads
profiling/results/, and none can validate a performance claim, because hosted runners have no GPU. The suite passing says the reference implementations agree with each other. It does not say the published numbers are right. That gap is #27, and stating it here keeps the badge honest.One Style bullet added. Leave
A,B,C,D,A_barandB_barin capitals.N803andN806are disabled inpyproject.tomldeliberately, so the names keep the spelling they have in the state-space literature. Without this note the next person to see those ignores may "fix" them and undo the decision.Also records that
mainis now protected and direct pushes are refused for maintainers too.Verification
The full gate, run locally at the pinned ruff:
python -m ruff check .reportsAll checks passed!python -m ruff format --check .reports 23 files already formattedpython -m pytestpasses, 25 testsCONTRIBUTING.mdis the only file changed. No source, no result data, no published figure.One check worth naming: this file contains fenced code blocks, and the lint job reformats fenced
pythonblocks in Markdown. The blocks here arebashand bare text, so the formatter leaves them alone, andruff format --checkconfirms it. This PR is also the first ordinary branch PR to run under the new workflow, so its own checks are part of the evidence.Not included
The commands are documented as broken rather than fixed. Fixing them is #48 and #36, and this PR does not touch code.