Add in unit tests and prototype for new config classes #336
Conversation
|
|
| super().__init__( | ||
| kwargs) | ||
|
|
||
| def check_hn_ground_truths(config,expected_ground_truth_ids:List[str]): |
There was a problem hiding this comment.
some of these things are also done in .validate(), I'd suggest you also call config.validate()
| ground_truth_ids=DEFAULT_PROSTATE_GROUND_TRUTH_IDS | ||
| config=ProstatePaper(num_structures=ground_truth_count) | ||
| check_hn_ground_truths(config, ground_truth_ids) | ||
|
|
There was a problem hiding this comment.
there should also be some similar tests for a "HeadAndNeckBase" and "ProstateBase" class that people should use
| ''' | ||
| # Number of structures to predict; if positive but less than the length of STRUCTURE_LIST, the relevant prefix | ||
| # of STRUCTURE_LIST will be predicted. | ||
| if (num_structures is None) or num_structures <= 0 or num_structures > len(STRUCTURE_LIST): |
There was a problem hiding this comment.
I'd fail if num_structures<=0 or >len(STRUCTURE_LIST).
| # by converting brainstem voxels to cord, as the latter is clinically more sensitive. | ||
| # We do the same to separate SPC and MPC; in this case, the direction of change is unimportant, | ||
| # so we choose SPC-to-MPC arbitrarily. | ||
| slice_exclusion_rules = [] |
There was a problem hiding this comment.
I would apply the same logic with kwargs.pop here. If they are set explicitly, use them, otherwise branch into the logic here. Also, logging.info should say that these are added automagically.
InnerEye/ML/config.py
Outdated
| if not self.disable_extra_postprocessing: | ||
| if self.slice_exclusion_rules is not None: | ||
| for rule in self.slice_exclusion_rules: | ||
| if rule.higher_class not in self.ground_truth_ids: |
There was a problem hiding this comment.
this would be better placed in an instance method "validate", similar to SummedProbRule
| with pytest.raises(Exception): | ||
| _ = HeadAndNeckBase( | ||
| ground_truth_ids=ground_truth_ids, | ||
| slice_exclusion_rules=slice_exclusion_rules) |
There was a problem hiding this comment.
I find it very important to check that it fails for the reason you expect. check that the raised exception has the expected message.
_ =... is very F#y, but not sure it's pythonic?
| with pytest.raises(ValueError): | ||
| _ = HeadAndNeckBase( | ||
| ground_truth_ids=ground_truth_ids, | ||
| summed_probability_rules=summed_probability_rules) |
There was a problem hiding this comment.
same here, check that it fails for the right reason
| with pytest.raises(ValueError): | ||
| _ = HeadAndNeckBase( | ||
| ground_truth_ids=ground_truth_ids, | ||
| summed_probability_rules=summed_probability_rules) |
| with pytest.raises(ValueError): | ||
| _ = HeadAndNeckBase( | ||
| ground_truth_ids=ground_truth_ids, | ||
| summed_probability_rules=summed_probability_rules) |
| """ | ||
| ground_truth_count = len(DEFAULT_HEAD_AND_NECK_GROUND_TRUTH_IDS) + 2 | ||
| with pytest.raises(ValueError): | ||
| _ = HeadAndNeckPaper(num_structures=ground_truth_count) |
| with pytest.raises(ValueError): | ||
| _ = HeadAndNeckPaper(num_structures=ground_truth_count, | ||
| ground_truth_ids_display_names=ground_truth_ids_display_names) |
| with pytest.raises(ValueError): | ||
| _ = HeadAndNeckPaper(num_structures=ground_truth_count, class_weights=class_weights) |
| with pytest.raises(ValueError): | ||
| _ = HeadAndNeckPaper(num_structures=ground_truth_count, fill_holes=fill_holes) |
| with pytest.raises(ValueError): | ||
| _ = HeadAndNeckPaper(num_structures=ground_truth_count, colours=colours) |
Split the existing HeadAndNeckBase into two classes. HeadAndNeckPaper contains the original functionality and HeadAndNeckBase is a new base class that has good defaults. Similarly ProstateBase has been split into ProstateBase and ProstatePaper. Deleted the unused ProstatePaperBase.
Please follow the guidelines for PRs contained here. Checklist:
Added/Changed/Removed/... in the "Upcoming" section.
and if needed a motivation why that change was required.