Skip to content

fix(medcat): prevent silent document loss in TestTrainSplitter.split()#574

Open
silverfoxdoc wants to merge 7 commits into
CogStack:mainfrom
silverfoxdoc:test_train_split_patch
Open

fix(medcat): prevent silent document loss in TestTrainSplitter.split()#574
silverfoxdoc wants to merge 7 commits into
CogStack:mainfrom
silverfoxdoc:test_train_split_patch

Conversation

@silverfoxdoc

Copy link
Copy Markdown
Contributor

Description

When using make_mc_train_test (or test_size > 0 in train_supervised_raw), TestTrainSplitter.split() seems to silently drop the majority of documents rather than assigning them to the train set. This results in a training set far smaller than expected.

Expected behaviour

When testing with real data set of 472 annotated documents and test_size=0.1, approximately 425 documents should be assigned to the train set and ~47 to the test set, given reasonably uniform numbers of annotations in each document; appreciate the method for splitting is more complex than simply raw document/annotation numbers.

Actual behaviour

Only ~20 documents appear in the train set. The remaining ~450 documents are neither assigned to train nor test set and are silently discarded.

Root cause

In medcat-v2/medcat/utils/data_utils.py, the split() method uses continue when the test quota is full, skipping documents entirely rather than routing them to the train set:

for i_document in np.random.permutation(range(0, num_of_docs)):
    if self.test_anns / self.total_anns >= self.test_size:
        continue  # document is silently dropped
    document = project['documents'][i_document]
    self._split_doc_train_test(document, cui_filter,
                               train_project, test_project)

Proposed fix

Add the document to the train set before continue:

for i_document in np.random.permutation(range(0, num_of_docs)):
    if self.test_anns / self.total_anns >= self.test_size:
        train_project['documents'].append(project['documents'][i_document])  # route to train
        continue
    document = project['documents'][i_document]
    self._split_doc_train_test(document, cui_filter,
                               train_project, test_project)

Tests

A new test class TestTrainSplitterNoDocumentLossTests has been added to
tests/utils/test_data_utils.py covering:

  • No documents lost — single project
  • No documents lost — multiple projects
  • Train set is larger than test set
  • Test set is not empty
  • make_mc_train_test wrapper also loses no documents

Tests were verified to fail on main (4/5 failing) and pass on this branch (5/5 passing).

Full transparency - the test was generated by claude AI with code reviewed manually to ensure performing as expected.

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.

1 participant