Skip to content
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

fix the ipython_display side effect #531

Merged
merged 11 commits into from
Dec 9, 2024
Merged

Conversation

timmysilv
Copy link
Collaborator

@timmysilv timmysilv commented Dec 5, 2024

User description

Context:
Objects with rich ipython displays do not render properly in interactive IPython

Description of the Change:

  • Default to the plain repr in interactive IPython
  • sneak in a workflow fix to avoid code injection via branch name. check the ultralytics issue shared in #software-learning if you're curious

Benefits:

  • No more funny HTML-like output for objects in interactive IPython

Possible Drawbacks:

  • Perhaps there's a way to show graphics nicely within IPython? this should suffice, and you can open a notebook if you want plotly

Related GitHub Issues:
Fixes #509


PR Type

Enhancement, Tests


Description

  • Enhanced _ipython_display_ methods across multiple modules to check for interactive IPython shell and default to print(self) for better display.
  • Added tests to verify the new behavior of _ipython_display_ methods in interactive IPython environments.
  • Updated changelog to reflect the changes made for IPython display enhancements.
  • Modified GitHub workflows to use environment variables for S3 path handling.

Changes walkthrough 📝

Relevant files
Enhancement
6 files
circuit_components.py
Improve IPython display handling for circuit components   

mrmustard/lab_dev/circuit_components.py

  • Added check for interactive IPython shell in _ipython_display_ method.
  • Default to print(self) for interactive shell.
  • +4/-0     
    dm.py
    Enhance IPython display for density matrix class                 

    mrmustard/lab_dev/states/dm.py

  • Added check for interactive IPython shell in _ipython_display_ method.
  • Default to print(self) for interactive shell.
  • +4/-0     
    ket.py
    Improve IPython display handling for ket class                     

    mrmustard/lab_dev/states/ket.py

  • Added check for interactive IPython shell in _ipython_display_ method.
  • Default to print(self) for interactive shell.
  • +4/-0     
    array_ansatz.py
    Enhance IPython display for array ansatz                                 

    mrmustard/physics/ansatz/array_ansatz.py

  • Added check for interactive IPython shell in _ipython_display_ method.
  • Default to print(self) for interactive shell.
  • +4/-0     
    polyexp_ansatz.py
    Improve IPython display handling for polyexp ansatz           

    mrmustard/physics/ansatz/polyexp_ansatz.py

  • Added check for interactive IPython shell in _ipython_display_ method.
  • Default to print(self) for interactive shell.
  • +4/-0     
    wires.py
    Enhance IPython display for wires class                                   

    mrmustard/physics/wires.py

  • Added check for interactive IPython shell in _ipython_display_ method.
  • Default to print(self) for interactive shell.
  • +4/-0     
    Tests
    4 files
    test_circuit_components.py
    Add tests for interactive IPython display in circuit components

    tests/test_lab_dev/test_circuit_components.py

    • Added test for interactive IPython shell display.
    +10/-0   
    test_array_ansatz.py
    Add tests for interactive IPython display in array ansatz

    tests/test_physics/test_ansatz/test_array_ansatz.py

    • Added test for interactive IPython shell display.
    +10/-0   
    test_polyexp_ansatz.py
    Add tests for interactive IPython display in polyexp ansatz

    tests/test_physics/test_ansatz/test_polyexp_ansatz.py

    • Added test for interactive IPython shell display.
    +10/-0   
    test_wires.py
    Add tests for interactive IPython display in wires class 

    tests/test_physics/test_wires.py

    • Added test for interactive IPython shell display.
    +14/-0   
    Documentation
    1 files
    CHANGELOG.md
    Update changelog for IPython display enhancements               

    .github/CHANGELOG.md

  • Updated changelog to include changes for interactive IPython display.
  • +3/-0     
    Configuration changes
    2 files
    tests_numpy.yml
    Update S3 path handling in numpy tests workflow                   

    .github/workflows/tests_numpy.yml

    • Added environment variable for reference name in S3 path.
    +3/-1     
    tests_tensorflow.yml
    Update S3 path handling in tensorflow tests workflow         

    .github/workflows/tests_tensorflow.yml

    • Added environment variable for reference name in S3 path.
    +3/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    codecov bot commented Dec 5, 2024

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 90.14%. Comparing base (2a1d7f9) to head (2c60938).
    Report is 1 commits behind head on develop.

    Additional details and impacted files
    @@             Coverage Diff             @@
    ##           develop     #531      +/-   ##
    ===========================================
    - Coverage    90.16%   90.14%   -0.02%     
    ===========================================
      Files          101      101              
      Lines         6291     6302      +11     
    ===========================================
    + Hits          5672     5681       +9     
    - Misses         619      621       +2     
    Files with missing lines Coverage Δ
    mrmustard/lab_dev/circuit_components.py 98.46% <100.00%> (+0.01%) ⬆️
    mrmustard/lab_dev/states/dm.py 95.65% <ø> (ø)
    mrmustard/lab_dev/states/ket.py 98.59% <ø> (ø)
    mrmustard/physics/ansatz/array_ansatz.py 100.00% <100.00%> (ø)
    mrmustard/physics/ansatz/polyexp_ansatz.py 98.60% <100.00%> (+0.01%) ⬆️
    mrmustard/physics/wires.py 99.30% <100.00%> (+0.01%) ⬆️
    mrmustard/widgets/__init__.py 98.14% <100.00%> (+0.05%) ⬆️

    ... and 1 file with indirect coverage changes


    Continue to review full report in Codecov by Sentry.

    Legend - Click here to learn more
    Δ = absolute <relative> (impact), ø = not affected, ? = missing data
    Powered by Codecov. Last update 2a1d7f9...2c60938. Read the comment docs.

    @timmysilv timmysilv marked this pull request as ready for review December 6, 2024 19:54
    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 6, 2024

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    509 - Fully compliant

    Compliant requirements:

    • Fixed display of lab_dev objects in interactive IPython shell by defaulting to plain repr
    • No need to manually call repr anymore as it's handled automatically
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🏅 Score: 95
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Duplication
    The interactive IPython check and print logic is duplicated across multiple files. Consider extracting this into a utility function.

    mrmustard/lab_dev/circuit_components.py Outdated Show resolved Hide resolved
    mrmustard/lab_dev/circuit_components.py Outdated Show resolved Hide resolved
    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 6, 2024

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    ✅ Add null check before accessing IPython shell instance to prevent runtime errors
    Suggestion Impact:The commit addressed the same issue of checking the IPython environment, but instead of adding a null check, it used a different approach by checking a variable 'mmwidgets.IN_INTERACTIVE_SHELL'.

    code diff:

    -        if isinstance(get_ipython(), InteractiveShell):
    +        if mmwidgets.IN_INTERACTIVE_SHELL:

    Check if get_ipython() returns None before checking its type to avoid potential
    AttributeError when running outside of IPython environment.

    mrmustard/lab_dev/circuit_components.py [728-730]

    -if isinstance(get_ipython(), InteractiveShell):
    +ipython = get_ipython()
    +if ipython is not None and isinstance(ipython, InteractiveShell):
         print(self)
         return
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential runtime error that could occur when the code runs outside of IPython environment. Adding a null check is crucial for robustness and preventing AttributeError exceptions.

    8

    💡 Need additional feedback ? start a PR chat

    @timmysilv timmysilv requested review from aplund and apchytr December 9, 2024 15:20
    Copy link
    Collaborator

    @apchytr apchytr left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Thanks Matt!

    @timmysilv timmysilv merged commit aeb346e into develop Dec 9, 2024
    8 checks passed
    @timmysilv timmysilv deleted the fix-ipython-terminal-repr branch December 9, 2024 20:05
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    Define multiple output methods for lab_dev
    2 participants