Adds exact journal matches to historical analysis #65
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JPrevost
requested review from
matt-bernhardt and
jazairi
and removed request for
matt-bernhardt
jazairi
approved these changes
Jul 30, 2024
| @@ -38,6 +39,11 @@ class Algorithms < ActiveSupport::TestCase | |||
| assert aggregate.pmid == 1 | |||
| end | |||
|
|
|||
| test 'journal exact counts are included in monthly aggregation' do | |||
| aggregate = Metrics::Algorithms.new.generate(DateTime.now) | |||
| assert aggregate.journal_exact == 1 | |||
There was a problem hiding this comment.
I worry a bit that this type of testing will lead to similar issues as we've seen in ETD (i.e.,adding a fixture breaks a bunch of tests). Not requesting a change, partly because I don't have any good suggestions and partly because I'd love to get this ticket to done.
There was a problem hiding this comment.
Yeah, you are definitely correct. As more fixtures are added, this test could fail.
I don't have a great idea that doesn't involve just not using fixtures or reimplementing the algorithm that calculates matches in the test itself... neither of which are really acceptable.
Thanks for pointing this out as I do think it's important for us to acknowledge when these "future" problems are being introduced.
Why are these changes being introduced: * Understanding our ability to detect search intent over time is core to our ability to know how we are doing Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TCO-54 How does this address that need: * Adds new field to track exact journal matches to Metrics::Algorithms * Updates Metrics::Algorithms to run journal exact matches in addition to the existing StandardIdentifer matches. This included a refactor to better support multiple match types. Document any side effects to this change: Journal exact matching is not guaranteed to be an indicator of user search intent because Journal names are also common words in many cases. When we build our our validation workflows, we'll be able to understand what percentage of these types of matches are definitely Journals and what percentage is ambiguous. We can likey update our algorithm to drop some of the more ambiguous detections at that point.
JPrevost
force-pushed
the
tco54-historical-snapshot-journals
branch
from
e43ae42 to
1e0fdc1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Why are these changes being introduced:
Relevant ticket(s):
How does this address that need:
Document any side effects to this change:
Journal exact matching is not guaranteed to be an indicator of user search intent because Journal names are also common words in many cases. When we build our our validation workflows, we'll be able to understand what percentage of these types of matches are definitely Journals and what percentage is ambiguous. We can likely update our algorithm to drop some of the more ambiguous detections at that point.