search

unibas-gravis / scalismo

Scalable Image Analysis and Shape Modelling
https://scalismo.org
Apache License 2.0
247 stars 65 forks source link

Error reading .h5 file #390

Closed Ghazi-Bouabene closed 2 years ago

Ghazi-Bouabene commented 2 years ago

Using version 0.91.1 I run into this exception when reading a legacy .h5 file

Exception in thread "main" java.lang.ClassCastException: class java.lang.String cannot be cast to class [Ljava.lang.String; (java.lang.String and [Ljava.lang.String; are in module java.base of loader 'bootstrap') at scalismo.io.HDF5Reader.$anonfun$readStringAttribute$1(HDF5Utils.scala:57) at scala.util.Try$.apply(Try.scala:210) at scalismo.io.HDF5Reader.readStringAttribute(HDF5Utils.scala:60) at example.ExampleApp$.main(ExampleApp.scala:147) at example.ExampleApp.main(ExampleApp.scala)

The representer name in the file is a pretty standard one (itkStandardMeshRepresenter).

My guess is that this is due to casting a String to an Array[String] here : https://github.com/unibas-gravis/scalismo/blob/18c327d8656c7f3f35e458b7ff42318222bee68a/src/main/scala/scalismo/io/HDF5Utils.scala#L59

I have tried a version where I cast to String instead and that works well. Is there a reason why we cast to Array here? (and also for other attribute types)

marcelluethi commented 2 years ago

Hi Ghazi,

Thanks for reporting this. What exactly do you mean by legacy .h5 file? Is a very old one that was still produced by statismo or another software? Does it work for other h5 files that you are workign with? I am asking because I am trying to figure out how severe this bug is.

The reason that we have Array[String] here is that this was required when using the other implementation of h5. As this case does not seem to be covered by our tests, I might have missed it.

Ghazi-Bouabene commented 2 years ago

Hi Marcel,

Thanks for the quick reply. The .h5 file was built by a customer of Shapemeans, most likely with the Statismo CLI. The issue does not seem to happen with .h5 files we generated with Scalismo.

Given that changing an already computed model is a least favorable option, should we consider dropping the cast to Array if it is no longer required by jhdf?

marcelluethi commented 2 years ago

Good to know that it only happens in legacy files.

Yes, we should fix this and also the subsequent method, where we have the same bug for attributes of type int. Once this is fixed, I can release a version 0.91.2. Do you want to do a PR?

Ghazi-Bouabene commented 2 years ago

sounds good. Will do a PR soon then.