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

CU-8692mevx8 Fix issue with filters not taking effect in train_supervised method #345

Merged
merged 2 commits into from
Sep 21, 2023

Conversation

mart-r
Copy link
Collaborator

@mart-r mart-r commented Sep 20, 2023

This is still a WIP.

But I identified an issue.

So it looks like when I made the change in #274 I had misunderstood the reason for the copying of the linking filters and "solved" it in a way that made the local filters not available for the vector context model or context based linker.

So effectively, the per-project filters and the extra CUI filters weren't taken into account at all.

So this PR will fix that.

With that said, there may be other issues in there. So I'd say this is still a WIP.

PS:
If someone wants to try this, they can install medcat based on this branch:

pip install https://github.com/mart-r/MedCAT/archive/CU-8692mevx8-supervised_train_filter.zip

Or to try and add it to an older version you can cherry-pick the change:

# Add my remote
git remote add mrfork git://github.com/mart-r/MedCAT
# Fetch
git fetch mrfork
# Cherry-pick commit
git cherry-pick 61c00f545392fce17d648210b51e8e0797e563a9

Or you can apply the patch

Copy link
Member

@tomolopolis tomolopolis left a comment

Choose a reason for hiding this comment

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

lgtm - anything to be added in the doc strings of train_supervised?

@mart-r
Copy link
Collaborator Author

mart-r commented Sep 21, 2023

anything to be added in the doc strings of train_supervised?

I don't think so. This should make it behave like it was supposed to to begin with.

@mart-r mart-r merged commit e0c6456 into CogStack:master Sep 21, 2023
5 checks passed
mart-r added a commit that referenced this pull request Sep 21, 2023
* remove bad merge <p> element

* CU-8692kpchc Fix for Rosalind link not working (#342)

* CU-8692kpchc Add the 403 exception to vocab downloader

* CU-8692kpchc Add the new vocab download link

* Add missing self argument (#343)

To `_refset_df2dict ` method in Snomed preprocessing

* CU-8692kn0yv Fix issue with fake dict in identifier based config

More specifically the get method which was not able to return default values for non-existant keys (#341)

* CU-8692mevx8 Fix issue with filters not taking effect in train_supervised method (#345)

* CU-8692mevx8 Fix issue with filters not taking effect in train_supervised method

* CU-8692mevx8 Fix filter retention in train_supervised method

---------

Co-authored-by: tomolopolis <tsearle88@gmail.com>
mart-r added a commit to mart-r/MedCAT that referenced this pull request Sep 21, 2023
…ised method (CogStack#345)

* CU-8692mevx8 Fix issue with filters not taking effect in train_supervised method

* CU-8692mevx8 Fix filter retention in train_supervised method
mart-r added a commit that referenced this pull request Sep 22, 2023
…ised method (#345)

* CU-8692mevx8 Fix issue with filters not taking effect in train_supervised method

* CU-8692mevx8 Fix filter retention in train_supervised method
mart-r added a commit that referenced this pull request Sep 22, 2023
…ised method (#345)

* CU-8692mevx8 Fix issue with filters not taking effect in train_supervised method

* CU-8692mevx8 Fix filter retention in train_supervised method
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