ekfac: fix compute_ekfac_ground_truth, add minimal CI #53
Conversation
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
This will make future diffs easier to read. Done using: uv run --with jupyter jupyter nbconvert --ClearOutputPreprocessor.enabled=True --inplace tests/ekfac_tests/compute_ekfac_ground_truth.ipynb
This class was removed in 8232b77 but the notebook code was not adapted.
This cell contains code from tests/ekfac_tests/test_covariance.py but fails with file not found errors and isn't actually needed in the rest of the notebook.
This makes it easier to run the script on a small GPU for testing.
This is adapted from the main branch of bergson.
|
|
… a bit more specific
|
Looks great! I made minor edits. One more thing before merge, I think we could maybe keep an original version as notebook. At least for now it might be still helpful for more closer debugging |
Use jupytext percent format which is directly interpretable by vscode (and can also be converted into ipynb using `jupytext --to notebook`). We want compute_ekfac_ground_truth to be: - Importable from other files. So it should be split in functions and importing it shouldn't have side-effects. - Usable a script we run from. So it should have a main that parses input arguments and run everything. - Usable as a notebook. So it should be split into cells where each cell can be executed individually and produce some output. To gain back usability as a notebook without compromising the other usecases, we split the logic that used to be in `main()` in multiple statements guarded by `if __name__ == "__main__"` at the end of the cell that defines the relevant function (since each of these guarded statement defines some variable, we actually need `or TYPE_CHECKING` to ensure they are visible to the typechecker).
All covariance-related cells are next to each other, same for all eigendecomposition-related code.
As discussed, I've added jupytext cell markers as an alternative, see 5e33a30 for the details and let me know if this seems usable to you. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
No description provided.