Skip to content

[Feature] Add upper bound parameter for min_max normalization technique #1431

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

Merged
merged 21 commits into from
Jul 18, 2025

Conversation

ryanbogan
Copy link
Member

@ryanbogan ryanbogan commented Jul 8, 2025

Description

Adds upper bound parameter for min_max normalization technique

Related Issues

Resolves #1210

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Ryan Bogan <[email protected]>
@ryanbogan
Copy link
Member Author

This PR is currently in draft state as I still need to add unit tests for the combination of lower and upper bound score calculations and integration tests for multiple different scenarios.

Copy link
Member

@martin-gaievski martin-gaievski left a comment

Choose a reason for hiding this comment

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

Is there a combined bounds test for scenarios with both upper and lower bounds applied simultaneously.

If it's not there please add something like this

@Test
public void testCombinedBounds_whenBothUpperAndLowerBounds_thenSuccessful() {
    Map<String, Object> params = Map.of(
        "lower_bounds", List.of(Map.of("mode", "apply", "min_score", 0.2)),
        "upper_bounds", List.of(Map.of("mode", "apply", "max_score", 0.8))
    );
    // test and assert
}

@ryanbogan
Copy link
Member Author

ryanbogan commented Jul 9, 2025

Is there a combined bounds test for scenarios with both upper and lower bounds applied simultaneously.

If it's not there please add something like this

@Test
public void testCombinedBounds_whenBothUpperAndLowerBounds_thenSuccessful() {
    Map<String, Object> params = Map.of(
        "lower_bounds", List.of(Map.of("mode", "apply", "min_score", 0.2)),
        "upper_bounds", List.of(Map.of("mode", "apply", "max_score", 0.8))
    );
    // test and assert
}

That's one of the cases that I still need to add, described in my comment above. Will add in a future commit

@ryanbogan ryanbogan changed the title [DRAFT] Add upper bound parameter for min_max normalization technique [Feature] Add upper bound parameter for min_max normalization technique Jul 10, 2025
@ryanbogan ryanbogan marked this pull request as ready for review July 10, 2025 18:07
@ryanbogan
Copy link
Member Author

Looking into failing tests

@ryanbogan
Copy link
Member Author

Rebase with main had caused a duplicate lower bounds clause to be added to the test search pipeline during the IT. Fixed now

Copy link

codecov bot commented Jul 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (24deed2) to head (373524c).

Additional details and impacted files
@@     Coverage Diff      @@
##   main   #1431   +/-   ##
============================
============================

☔ 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.

Copy link
Member

@martin-gaievski martin-gaievski 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, thank you @ryanbogan

Copy link
Member

@owaiskazi19 owaiskazi19 left a comment

Choose a reason for hiding this comment

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

Looks clean. Few comments

@owaiskazi19 owaiskazi19 self-requested a review July 11, 2025 19:22
Copy link
Member

@martin-gaievski martin-gaievski 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, thanks Ryan

Copy link
Member

@owaiskazi19 owaiskazi19 left a comment

Choose a reason for hiding this comment

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

Looks much cleaner with a minor comment

Signed-off-by: Ryan Bogan <[email protected]>
Copy link
Member

@owaiskazi19 owaiskazi19 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. Thanks for addressing comments

@ryanbogan ryanbogan closed this Jul 18, 2025
@ryanbogan ryanbogan reopened this Jul 18, 2025
@heemin32 heemin32 merged commit 5baff47 into opensearch-project:main Jul 18, 2025
81 of 116 checks passed
@ryanbogan ryanbogan deleted the upper_bound_hybrid branch July 18, 2025 23:07
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.

[FEATURE] Add upper_bound in min-max normalization
4 participants