ITK icon indicating copy to clipboard operation
ITK copied to clipboard

WIP: Fix for wrong cell type when LINE and POLY_LINE present

Open PranjalSahu opened this issue 1 year ago • 4 comments

PR Checklist

  • [ ] No API changes were made (or the changes have been approved)
  • [ ] No major design changes were made (or the changes have been approved)
  • [ ] Added test (or behavior not changed)
  • [ ] Updated API documentation (or API not changed)
  • [ ] Added license to new files (if any)
  • [ ] Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • [ ] Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for further development details if necessary.

PranjalSahu avatar Aug 03 '22 21:08 PranjalSahu

@jhlegarreta Yes, I am adding those. It fails for the existing test mesh when writing in binary format. Also, the test mesh has pixel data that needs to be corrected. For color scalar data in binary mode writing, we convert to uchar which is making the current data value from 0.51 to 0. That data value has to be made uint8.

I assigned the value to cellType variable which permanently modified it for the rest of the loop.

PranjalSahu avatar Aug 04 '22 13:08 PranjalSahu

@jhlegarreta Yes, I am adding those. It fails for the existing test mesh when writing in binary format. Also, the test mesh has pixel data that needs to be corrected. For color scalar data in binary mode writing, we convert to uchar which is making the current data value from 0.51 to 0. That data value has to be made uint8.

I assigned the value to cellType variable which permanently modified it for the rest of the loop.

Thanks Pranjal. So maybe that explains why I many of the tests for the mesh IO classes were not working as expected, and I ended up by commenting those lines: https://github.com/InsightSoftwareConsortium/ITK/pull/3403#issuecomment-1141132531

Would be great if this was the fix that made those work.

jhlegarreta avatar Aug 04 '22 13:08 jhlegarreta

@jhlegarreta I am also testing the commented test cases. Some failures should not happen like a change in the number of points.

PranjalSahu avatar Aug 04 '22 16:08 PranjalSahu

On a slightly related note, maybe the Valgrind issues that were uncovered by some of the mentioned tests are related to this too: https://open.cdash.org/viewDynamicAnalysis.php?buildid=8084453

jhlegarreta avatar Aug 06 '22 14:08 jhlegarreta

ghostflow-check-master is stuck. I have re-pushed now.

PranjalSahu avatar Aug 19 '22 01:08 PranjalSahu

@bradking @mathstuf do you have some advice regarding ghostflow here? It is obviously misbehaving.

dzenanz avatar Aug 19 '22 13:08 dzenanz

Do: check

mathstuf avatar Aug 19 '22 14:08 mathstuf