Closed keileg closed 7 months ago
@keileg thank you for enforcing types in topological information. The only suggestion I have is to implement a test that checks that the topological information is indeed an integer for all the grids.
I'm not sure if I agree: The attributes will be int
s now and remain so unless we remove that line, which is highly unlikely. Moreover, it is not a disaster if the attributes become floats again - the previous code worked perfectly fine for several years. I realize that sparse find etc. may be less stable for floats, but again, this will not happen unless we revert the code.
So, I would say the benefits of testing this feature do not warrant the cost of dealing with extra tests. Do you agree, or have I missed something?
@keileg thank you for enforcing types in topological information. The only suggestion I have is to implement a test that checks that the topological information is indeed an integer for all the grids.
I'm not sure if I agree: The attributes will be
int
s now and remain so unless we remove that line, which is highly unlikely. Moreover, it is not a disaster if the attributes become floats again - the previous code worked perfectly fine for several years. I realize that sparse find etc. may be less stable for floats, but again, this will not happen unless we revert the code.So, I would say the benefits of testing this feature do not warrant the cost of dealing with extra tests. Do you agree, or have I missed something?
In addition to being a suggestion, keep in mind that tracking types correctly correlates with code stability. Nevertheless, I agree with you that as long as users do not mess with types associated with topology, these changes are sufficient.
Proposed changes
Updates:
Types of changes
What types of changes does this PR introduce to PorePy? Put an
x
in the boxes that apply.Checklist
Put an
x
in the boxes that apply or explain briefly why the box is not relevant.pytest
was run with the--run-skipped
flag.