Skip to content

Conversation

@RaghavaAlajangi
Copy link
Member

@RaghavaAlajangi RaghavaAlajangi commented Oct 21, 2025

This PR aims to implement issue #210

  • Implement logic
  • CICD passed
  • Update CHANGELOG

@codecov
Copy link

codecov bot commented Oct 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.32%. Comparing base (f227e20) to head (b07dfdd).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #222      +/-   ##
==========================================
- Coverage   78.39%   77.32%   -1.08%     
==========================================
  Files          67       67              
  Lines        7671     7681      +10     
==========================================
- Hits         6014     5939      -75     
- Misses       1657     1742      +85     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@RaghavaAlajangi
Copy link
Member Author

Hi Paul,
Do we need a test function here?

@paulmueller
Copy link
Member

Hi Paul, Do we need a test function here?

Yes, please add two tests:

  • load two datasets and create a zoomed-in contour plot
  • load a dataset and create a contour plot with contour levels that don't make sense (e.g. at 100% of KDE)

@RaghavaAlajangi
Copy link
Member Author

Hi Paul,
let me know before accepting the changes. I will update the changelog.

@paulmueller paulmueller changed the base branch from main to main2 January 2, 2026 14:41
@paulmueller paulmueller changed the base branch from main2 to main January 2, 2026 14:41
Copy link
Member

@paulmueller paulmueller left a comment

Choose a reason for hiding this comment

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

Just a few minor things to harden the tests.

# Verify that zoom was applied
assert np.all(np.isfinite(x_range))
assert np.all(np.isfinite(y_range))
assert x_range[0] != 0 or x_range[1] != 1
Copy link
Member

Choose a reason for hiding this comment

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

Please test for actual values of the range (the zoomed-in values). A future regression might pass this test even though it breaks the functionality.

# Set contour percentiles to extreme values (edge cases)
# 100% percentile is at the maximum KDE value
pv.doubleSpinBox_perc_1.setValue(100.0) # Maximum percentile
pv.doubleSpinBox_perc_2.setValue(99.0) # Near maximum
Copy link
Member

Choose a reason for hiding this comment

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

Please set the second level to 100% as well. The test is about catching error messages when nonsense is plotted.


# Get the plot state
plot_state = mw.pipeline.get_plot(plot_id).__getstate__()

Copy link
Member

Choose a reason for hiding this comment

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

Please add an assert that makes sure there is only one plot.

@RaghavaAlajangi
Copy link
Member Author

Hi Paul,
I made the changes that you asked for. Could you have alook?

@paulmueller paulmueller merged commit d740fb4 into DC-analysis:main Jan 7, 2026
4 of 5 checks passed
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