- 
                Notifications
    You must be signed in to change notification settings 
- Fork 37
          Enable type-checking for visualization.py
          #347
        
          New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
  
    Enable type-checking for visualization.py
  
  #347
              Conversation
This commit adds type annotations to `visualization.py`, along with its corresponding tests, enabling full type-checking with mypy while preserving existing functionality. **Related issue:** This PR continues the work started in TeamGraphix#302, TeamGraphix#308, and TeamGraphix#312.
| Codecov Report❌ Patch coverage is  
 Additional details and impacted files@@            Coverage Diff             @@
##           master     #347      +/-   ##
==========================================
+ Coverage   79.52%   85.86%   +6.34%     
==========================================
  Files          41       42       +1     
  Lines        5793     5704      -89     
==========================================
+ Hits         4607     4898     +291     
+ Misses       1186      806     -380     ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually we should update the docstrings for consistency too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Do we want to change all the figsize variables to _Point = tuple[float, float] type?
- Line 768 do we want to give a more descriptive ValueError?
- Additional bug noticed in GraphVisualizer init . If no measurement planes input, generated measurement planes include measurements on output nodes. I will make an issue for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that even if figsize could formally be of type _Point, it makes sense to distinguish them since a size is semantically different than a point. The two other issues should have been fixed by the refactoring. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to approve once comments are checked.
| I just refactored the visualization module: the behavior should be unchanged, and the code logic is the same, but I reduced code duplication and improved test coverage. | 
This commit adds type annotations to
visualization.py, along with its corresponding tests, enabling full type-checking with mypy while preserving existing functionality.Related issue:
This PR continues the work started in #302, #308, and #312.