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

Some update #2355

Merged
merged 11 commits into from
Aug 26, 2024
Merged

Some update #2355

merged 11 commits into from
Aug 26, 2024

Conversation

chunyuma
Copy link
Collaborator

This PR conducts the following:

  1. Merge the recent update on the ARAX's algorithm from a closed PR that wasn't merged into the master branch.
  2. Update the code/config_dbs.json to change the path location of new xDTD and xCRG database
  3. Modify some tests for test_ranker.py

@chunyuma chunyuma requested review from dkoslicki and edeutsch August 26, 2024 00:21
@dkoslicki
Copy link
Member

Were the ranking results checked against the list from #2300? I.e. the comparison from here showed the new ranker was doing pretty well. Are the ranker changes in this PR already included in that? If not, it would be prudent to at least spot-check the ranker (better yet, run it through the benchmark repo) given these changes.

@chunyuma
Copy link
Collaborator Author

chunyuma commented Aug 26, 2024

@dkoslicki, the change in this ranker script is not significantly different from the one in #2300, but just includes a looping algorithm proposed by Eric (see #2334). As I mentioned in Slack, it has passed almost all ranker tests but only failed on test_ARAXRanker_test9_asset619, test_ARAXRanker_test9_asset623, test_ARAXRanker_test13_asset355. I have listed the reason there. I think it is sufficient to demonstrate its validity, right?

@chunyuma
Copy link
Collaborator Author

BTW, reason of those three failed tests are not due to the change in the algorithm.

For test_ARAXRanker_test13_asset355, it seems to be due to the change in Node Synonymizer.

For test_ARAXRanker_test9_asset619 and test_ARAXRanker_test9_asset623, I am not sure if they are due to the latest version of xDTD. But based on what I have checked on the old results. the target drug results don't contain any inferred edge from xDTD so I guess they might come from the lookup function. However, the failed tests don't find the target results in the returned list. I am wondering if it is due to the KG2.10.0 problem.

@dkoslicki
Copy link
Member

Yes, @chunyuma that does seem to be sufficient. I wanted to be sure this didn't negatively affect the good work you've done with the ranker. And the other issues seem non-ranker, non-xDTD, so I'll approve the PR

@chunyuma chunyuma merged commit e979e60 into master Aug 26, 2024
4 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.

3 participants