Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adding search processor for score normalization and combination #227
Adding search processor for score normalization and combination #227
Changes from 6 commits
3ebe720
71c92bf
328620f
77d6eec
150ff33
9e1c8ba
729513b
01590ed
4d1e03f
74b23b2
29b397f
069fea2
e746e12
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a unit test where count is zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack, I'll add more tests in next PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Umm. I thought that divide by zero will throw an exception but I was wrong. For floating point number, it will be Infinity, -Infinity, or NaN. If those values are okay, I am fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In such case we need to return 0.0, as this is the case when, if there are scores, we're skipping some of them and not increasing count. Let me update the method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this enum really necessary? Why not just pass the
ArithmeticMeanScoreCombinationMethod
instance itself and callmethod.combine
directly instead of wrapping it with enum?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String for technique passed by user at factory level, we can pass enum variable safer between that many layers. Plus it's a single place that encapsulates technique and method abstractions. I switched to this approach after trying technique and method or string and then method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't all you need is just calling combine method in the end?
Then, can we just pass
ArithmeticMeanScoreCombinationMethod
instance at factory level? I don't think passing instance is not safer than passing enum.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my concern is more to keep factory light. We're going to add more methods and other configurations (like weights for sub-queries) later, and in such case factory became more responsible for kicking off calculations, while it's intended design was to parse user input and construct processor. Enum layer is taking that preparation for calculations responsibility so factory can stay light
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to convert string to enum in factory anyway. The suggested method is same, you just need to convert string to instance in factory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is that now it's a simple conversion String -> enum, and it's same as String -> method. But with planned next PR changes (and we know we'll be doing this as per HLD RFC) it will be string + map_of_params -> method, and I'd like to keep it String -> enum. And also to keep knowledge of the actual computation method away from the factory and keep it at lower levels, factory knows just the technique, technique knows about method specifics .