Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Partial ground truth inference PR (465) missed two things #522

Merged
merged 4 commits into from
Jul 6, 2021

Conversation

dumbledad
Copy link
Contributor

@dumbledad dumbledad commented Jul 6, 2021

There were two things missing/wrong in PR #465:

  1. I forgot to update the documentation.
  2. The unit test test_innereyecontainer_setup_passes_on_allow_incomplete_labels only tested the first call to the mock function mocked_convert_channels_to_file_paths, subsequent calls in the loop were not reached.

Further fixes to #459

  • Ensure that your PR is small, and implements one change. No, it implements two small changes.
  • Add unit tests for all functions that you introduced or modified.
  • Run PyCharm's code cleanup tools on your Python files. I am using VSCode.
  • Link the correct GitHub issue for tracking.
  • Update the Changelog file: Describe your change in terms of
    Added/Changed/Removed/... in the "Upcoming" section.
  • When merging your PR, replace the default merge message with a description of your PR,
    and if needed a motivation why that change was required.

Tim Regan added 2 commits July 6, 2021 08:09
My approach was wrong, since it only tested the first time the mocked function was called.
I've swapped to Anton's suggestion by adding return values so that the mock can be called
multiple times in a test execution.
@dumbledad dumbledad added the no changelog needed CHANGELOG.md does not need to be updated in this PR label Jul 6, 2021
@dumbledad dumbledad self-assigned this Jul 6, 2021
@dumbledad dumbledad marked this pull request as ready for review July 6, 2021 07:38
@dumbledad dumbledad requested review from ant0nsc and Shruthi42 July 6, 2021 07:39
@dumbledad dumbledad enabled auto-merge (squash) July 6, 2021 07:41
@dumbledad dumbledad removed the no changelog needed CHANGELOG.md does not need to be updated in this PR label Jul 6, 2021
@dumbledad dumbledad changed the title Timregan/459 partial ground truth missed two things Partial ground truth inference PR (465) missed two things Jul 6, 2021
Shruthi42
Shruthi42 previously approved these changes Jul 6, 2021
Mocks are matched by function name string which is very fragile. If the function gets renamed
this will fail prompting the renamer to fix the test.
@dumbledad dumbledad merged commit 3fc71b8 into main Jul 6, 2021
@dumbledad dumbledad deleted the timregan/459-partial-ground-truth-missed-two-things branch July 6, 2021 10:18
@javier-alvarez
Copy link
Contributor

No linked issue

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow segmentation model inference to run on datasets without segmentations or partial segmentations
4 participants