Fix types so pylance's type checker doesn't complain#189
Fix types so pylance's type checker doesn't complain#189curtischong wants to merge 4 commits intomainfrom
Conversation
| def test_infer_sim_state_property_scope(si_sim_state: SimState) -> None: | ||
| """Test inference of property scope.""" | ||
| scope = infer_property_scope(si_sim_state) | ||
| assert set(scope["global"]) == {"pbc"} |
There was a problem hiding this comment.
here it is global because things like the PBC cannot be different for things in a batched state. per_batch is the misleading one here as that it the per_graph level of abstraction. If pylance doesn't like global I would agree that changing it is a good thing to do but I would think that a different name, for example shared, might be better. I would also consider renaming per_batch as I don't think that per_batch is clear that it's per substate within a batched simstate.
CompRhys
left a comment
CompRhys
left a comment
There was a problem hiding this comment.
per_graph is the wrong label but not opposed to changing the name here to avoid linting/language server issues.
|
Closing since the per_graph change is more involved and I want to roll it back but still keep Rhys' comments. I'll put up a smaller initial PR here: #191 I'll prob rename per_batch -> per_graph in its own isolated PR |
|
Agree with Rhys that if you open another PR I'd favor "shared" or "per-state" over "per-graph". Fine switching away from global if it's causing issues. Alternate ideas: "atomwise" - "batchwise" - "shared". Could also make an Enum so it's not just strings. |
3 participants
Summary
Checklist
Before a pull request can be merged, the following items must be checked:
Run ruff on your code.
We highly recommended installing the pre-commit hooks running in CI locally to speedup the development process. Simply run
pip install pre-commit && pre-commit installto install the hooks which will check your code before each commit.