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 #961: Removed old visualization code from evaluate.py #965

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Bhavya1604
Copy link

@Bhavya1604 Bhavya1604 commented Mar 10, 2025

I have removed the old visualization code from evaluate.py, as requested in issue #961

The changes include:
Removing root_dir from evaluate_image_boxes and evaluate_boxes.
Eliminating unnecessary visualize.plot_predictions and cv2.imwrite calls.
Ensuring all related functions remain functional without visualization dependencies.

All tests are passing, except for an unrelated JSON test.

Looking forward to your feedback!

Thanks!

Copy link
Contributor

@henrykironde henrykironde left a comment

Choose a reason for hiding this comment

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

Thanks @Bhavya1604 for getting to work on this PR. I expected some changes in the tests too.

@Bhavya1604
Copy link
Author

Bhavya1604 commented Mar 10, 2025

@henrykironde Thanks for the feedback! I’m working on updating the tests and will push the changes soon.

@Bhavya1604 Bhavya1604 requested a review from henrykironde March 10, 2025 20:28
@Bhavya1604
Copy link
Author

I have made the following changes:

  1. Removed root_dir from evaluate.py.
  2. Removed all old visualization code from evaluate.py.
  3. Updated the corresponding tests:
    • test_evaluate.py
    • test_main.py
  4. Removed root_dir from def evaluate in main.py.

All tests have passed on my local system.

Let me know if any further changes are needed!

Copy link
Contributor

@henrykironde henrykironde left a comment

Choose a reason for hiding this comment

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

There are just a few things left. An I have commented on one. The other is running yapf to clean up the code. That is why it is failing

@@ -113,9 +106,8 @@ def test_point_recall_image(root_dir, tmpdir):
img_path = get_data("OSBS_029.png")
if root_dir == "tmpdir":
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still root_dir here?

Copy link
Author

Choose a reason for hiding this comment

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

No, it's not required. I missed it thanks for catching that! I'll update it now.

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.

2 participants