Skip to content

Conversation

@wyli
Copy link
Contributor

@wyli wyli commented Apr 1, 2021

update tests (TEST_CASE_5 duplicated), remove json loading (#1878 (comment))

Status

Ready

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).

Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli wyli requested review from Nic-Ma and bhashemian April 1, 2021 07:53
@wyli
Copy link
Contributor Author

wyli commented Apr 1, 2021

/black

@bhashemian
Copy link
Member

bhashemian commented Apr 1, 2021

Hi @wyli, why are you removing the json loading part? For pathology, since this metric relies on all the ground truth masks and all the probability masks, it should be called independently after all the inferences are done, and they need be defined in a json file.

@wyli
Copy link
Contributor Author

wyli commented Apr 1, 2021

Hi @wyli, why are you removing the json loading part? For pathology, since this metric relies on all the ground truth mask and all the probability masks, it should be called independently after all the inferences are done, and they need be defined in a json fie.

yes before the data working group provides a concrete solution we shouldn't couple the lesion evaluation module with json encoding/decoding logic. if it's really needed please write a separate json loader. This class should only work with Sequence[Dict]. my previous comment was somehow ignored... (#1878 (comment))

Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>
@wyli
Copy link
Contributor Author

wyli commented Apr 1, 2021

thanks for the update @behxyz! there's an example of deepgrow with a demo script to show a task-specific json loader https://github.com/Project-MONAI/tutorials/blob/55006d748626f6af59567a295725ccbd3bdd6bef/deepgrow/ignite/train.py#L315

@bhashemian
Copy link
Member

Hi @wyli, why are you removing the json loading part? For pathology, since this metric relies on all the ground truth mask and all the probability masks, it should be called independently after all the inferences are done, and they need be defined in a json fie.

yes before the data working group provides a concrete solution we shouldn't couple the lesion evaluation module with json encoding/decoding logic. if it's really needed please write a separate json loader. This class should only work with Sequence[Dict]. my previous comment was somehow ignored... (#1878 (comment))

OK, I'll add a separate json reader till we have an agreed upon solution. Would this be the topic of data WG or pathology WG?

@bhashemian
Copy link
Member

I updated the test case number to be in an order and committed to your PR.

@bhashemian bhashemian enabled auto-merge (squash) April 1, 2021 14:20
@bhashemian bhashemian merged commit debc561 into Project-MONAI:master Apr 1, 2021
@wyli wyli deleted the followup-1878 branch April 1, 2021 16:50
nsrivathsa pushed a commit to nsrivathsa/MONAI that referenced this pull request Apr 12, 2021
* followup of Project-MONAI#1878, fixes tests, remove json loading

Signed-off-by: Wenqi Li <wenqil@nvidia.com>

* Update test ordinal numbers

Signed-off-by: Behrooz <3968947+behxyz@users.noreply.github.com>

Co-authored-by: Behrooz <3968947+behxyz@users.noreply.github.com>
Signed-off-by: Neha Srivathsa <nsrivathsa@nvidia.com>
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