dcmjs-org / dcmjs

Javascript implementation of DICOM manipulation
https://dcmjs.netlify.com/
MIT License
292 stars 111 forks source link

Fix/263 large objects #263 #267

Closed wayfarer3130 closed 2 years ago

wayfarer3130 commented 2 years ago

Fix large objects being created by the addAccessor. The problem was that an accessor was added for a private creator tag, which ends up being an integer, so it extended the array to the value of the private tag. This caused a huge number of nulls to be created. The solution was to use a proxy rather than accessors, to avoid actually having the values be native accessors, but done as a request time call. This also reduces the memory footprint overall.

wayfarer3130 commented 2 years ago

@pieper - I modified the approach. I think it is a better approach overall, as it is a lower memory footprint. Added some extra tests to cover a few more things.

pieper commented 2 years ago

@pieper - I modified the approach. I think it is a better approach overall, as it is a lower memory footprint. Added some extra tests to cover a few more things.

Great, thanks @wayfarer3130 👍

@dbousamra can check if this fixes #263 for you?

dbousamra commented 2 years ago

@pieper Not easily no. I will need to wait for a release. If that sample test case I provided passes, I am sure it will be fine

pieper commented 2 years ago

@wayfarer3130 are you able to test with @dbousamra 's example data? Alternatively, are you sure enough of the fix that we should just do a release?

wayfarer3130 commented 2 years ago

@pieper I added @dbousamra test data as a unit test, after deleting the patient name details. It was failing before the fix and passing afterwards, so I'm reasonably confident this fixes things. I also added a couple of other tests to try to get more coverage on this area as it is clearly something that is tricky to get right.

pieper commented 2 years ago

Thanks @wayfarer3130. @dbousamra please test when you can.

github-actions[bot] commented 2 years ago

:tada: This PR is included in version 0.22.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: