Roundtrip test and some fixes to inconsistencies between the schema, examples, and JSONMetadataHandler #101
Conversation
This one is from https://github.com/ChannelIQ/jsoncompare with minor modification submitted in this PR ChannelIQ/jsoncompare#8. Without the modification, it is required that the compared JSONs have identical number of keys, which is not logical, since we are specifically interested in ignoring specified keys. Related to QIICR#97
since this is not a DICOM attribute, it should start from lower case
The test is failing at the moment, because of the identified inconsistencies, this will need to be fixed. Addresses QIICR#97.
|
@fedorov Please check if all your commits (except of the last one for adding the missing file) are there. |
|
Did you expect the test would fail too? It sounded from the earlier communication that you expect they should pass, so I wanted to confirm. |
|
I only wanted to correct the syntax of the test. I didn't mean or check anything else. But I can go ahead and check what's wrong. |
|
ah, ok - makes sense. Can you take a look how much effort would be to fix inconsistencies? I didn't plan to merge until the test passes. |
|
@fedorov I figured out, that the DICOM-attributes of https://github.com/QIICR/dcmqi/blob/master/data/ct-3slice/liver.dcm Instead of using the liver.dcm from the repository, we should use the output from itkimage2segimage otherwise the roundtrip would not make sense because we don't continue working with the produced data, right? |
|
Good observation! It probably makes sense to run itk2seg first using baseline metadata, then run seg2itk on the output of itk2seg, not baseline seg, and use the output metadata file from seg2itk to compare with the baseline json that was used in the itk2seg in the first step. Does this make sense? |
|
I propose to do the following:
I would set dependencies between the tests in order to have the right order when running them. |
|
makes sense ;) |
- added missing attributes - fixed json comparison script (for the reference ChannelIQ/jsoncompare#8 and ChannelIQ/jsoncompare#9)
|
@fedorov ready for merge |
|
Looks great! One question - can you write up a summary of the changes you did to jsoncompare, and consider submitting pull request to the upstream? |
|
@fedorov I already submitted a PR to the upstream just before I created the PR here. |
|
Very good - I didn't check that repo, so thanks for adding a reference here. |
|
In my last commit message I added a reference to your PR and my PR of jsoncompare upstream |
|
Sorry, I missed it |
Followup on #97.
jsoncompare is from https://github.com/ChannelIQ/jsoncompare, with ChannelIQ/jsoncompare#8 applied.