Skip to content

Inversion code clean up#3881

Open
jellybean2004 wants to merge 12 commits intomainfrom
isn_rework
Open

Inversion code clean up#3881
jellybean2004 wants to merge 12 commits intomainfrom
isn_rework

Conversation

@jellybean2004
Copy link
Member

@jellybean2004 jellybean2004 commented Feb 19, 2026

Description

This PR cleans up the code for the Inversion Perspective in the qtgui directory, which includes:

  • ruff check and formatting
  • type hinting
  • proper doc string formatting
  • removal of commented code

This also includes some work on invertor.py, #2974

How Has This Been Tested?

Manually ran SasView to test functionality is unchanged.

Review Checklist:

Documentation

  • There is nothing that needs documenting
  • Documentation changes are in this PR
  • There is an issue open for the documentation (link?)

Installers

  • There is a chance this will affect the installers, if so
    • Windows installer (GH artifact) has been tested (installed and worked)
    • MacOSX installer (GH artifact) has been tested (installed and worked)
    • Wheels installer (GH artifact) has been tested (installed and worked)

Licensing

  • The introduced changes comply with SasView license (BSD 3-Clause)

@jellybean2004 jellybean2004 changed the title Isn rework Inversion code clean up Feb 20, 2026
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR performs code cleanup for the Inversion Perspective in the qtgui directory, applying ruff formatting, adding type hints, updating docstrings, and removing commented code. The changes are primarily cosmetic and aim to improve code quality without altering functionality.

Changes:

  • Added type hints to method signatures across all modified files
  • Updated docstrings to follow proper formatting conventions
  • Applied ruff code formatting (spacing, quote consistency, line breaks)
  • Removed commented code sections

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
InversionPerspectiveTest.py Added type hints to test methods and fixtures; updated docstrings to single-line format; changed class name from lowercase to PascalCase
Thread.py Added comprehensive type hints for thread classes; reformatted method signatures for better readability; updated docstrings
InversionUtils.py Reformatted WIDGETS enum for improved readability with one item per line
InversionPerspective.py Added type hints to methods; updated docstrings; improved multiline string formatting; removed commented code
InversionLogic.py Added type hints; updated docstrings; improved code formatting and spacing
DMaxExplorerWidget.py Added type hints; updated docstrings; improved code formatting and error message structure

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@DrPaulSharp DrPaulSharp left a comment

Choose a reason for hiding this comment

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

Looks good so far, a couple of points to address alongside the remaining copilot comments.

@jellybean2004
Copy link
Member Author

@DrPaulSharp, I have rebased this on main. Also did some work on invertor.py.

One thing I wanted to check - the regularisation constant is called alpha in the code. There is a TODO asking why this is so. I did have a quick go at renaming it reg. This will require changing quite a few calls and references across different files. Is it worth renaming the variable, or should we just stick with alpha?

@DrPaulSharp
Copy link
Contributor

@DrPaulSharp, I have rebased this on main. Also did some work on invertor.py.

One thing I wanted to check - the regularisation constant is called alpha in the code. There is a TODO asking why this is so. I did have a quick go at renaming it reg. This will require changing quite a few calls and references across different files. Is it worth renaming the variable, or should we just stick with alpha?

I think it's probably just worth leaving for now. Unless there is a compelling need to change the name it doesn't seem worth doing that much work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants