Skip to content

fix gui #87

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix gui #87

wants to merge 2 commits into from

Conversation

c-schicho
Copy link

currently, the pipeline argument is required although using the gui it is expected to be set there. furthermore, the settings set in the gui are not fully considered in the executed pipeline.

this pull request removes the required flag from the argument but adds also validation to ensure it is correctly set. furthermore, it adapts the parsed arguments to correctly use the parameters set in the gui.

let me know if you wanna fix this differently.

@franioli franioli requested a review from Copilot July 2, 2025 12:32
Copy link

@Copilot 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 makes the --pipeline argument optional when using the GUI, enforces validation otherwise, and ensures GUI settings propagate through the CLI parser into the configuration.

  • parser.py: Removed required=True for --pipeline, added validation when --gui is not set, and mapped GUI outputs into individual args (extractor, matcher) instead of config_file.
  • gui.py: Extracted argument-assembly logic into a private helper and updated on_submit/get_values to use it, mapping GUI selection to extractor/matcher configs.
  • config.py: Added a GUI branch to pick extractor/matcher directly from parsed GUI args and adjusted validation logic accordingly.

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/deep_image_matching/parser.py Make --pipeline optional under GUI, add CLI validation, map new GUI args
src/deep_image_matching/gui.py Refactor argument assembly into __get_args, update submission methods
src/deep_image_matching/config.py Branch config parsing on GUI mode for extractor/matcher selection
Comments suppressed due to low confidence (3)

src/deep_image_matching/gui.py:72

  • [nitpick] Rename __get_args to _get_args (single underscore) to avoid Python name-mangling and align with common private method conventions.
    def __get_args(self):

src/deep_image_matching/parser.py:148

  • Add a unit test for this validation branch to verify that an error is raised when --gui is false and no --pipeline is provided.
    if not args.gui and args.pipeline is None:

src/deep_image_matching/parser.py:153

  • Remove the redundant args.gui = True assignment—args.gui is already set by the CLI parser.
        args.gui = True

@@ -285,14 +285,21 @@ def __init__(self, args: dict):
Args:
args (dict): The input arguments provided by the user.
"""
is_gui = args["gui"] is not None and args["gui"]
Copy link
Preview

Copilot AI Jul 2, 2025

Choose a reason for hiding this comment

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

Simplify the GUI flag check to is_gui = args["gui"] since args["gui"] is always defined.

Suggested change
is_gui = args["gui"] is not None and args["gui"]
is_gui = args["gui"]

Copilot uses AI. Check for mistakes.

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.

1 participant