Skip to content

followup of #1878 #1913

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

Merged
merged 3 commits into from
Apr 1, 2021
Merged

followup of #1878 #1913

merged 3 commits into from
Apr 1, 2021

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).

@wyli wyli requested review from bhashemian and Nic-Ma 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))

@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 <[email protected]>

* Update test ordinal numbers

Signed-off-by: Behrooz <[email protected]>

Co-authored-by: Behrooz <[email protected]>
Signed-off-by: Neha Srivathsa <[email protected]>
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