CLIMADA-project / climada_python

Python (3.8+) version of CLIMADA
GNU General Public License v3.0
296 stars 117 forks source link

Impact read hdf5 fails if hazard event name is not a string #807

Closed chahank closed 1 month ago

chahank commented 9 months ago

Similarly to issue #789 , saving an impact to hdf5 if the hazard name is not a list of strings causes issues when the data is read again.

Precisely, the attribute impact.event_name must be a list of strings at the moment of saving to hdf5 impact.to_hdf5, or the reading method impact.from_hdf5 should be able to handle non-numerical values.

peanutfun commented 9 months ago

This is not an issue with writing, but with reading: https://github.com/CLIMADA-project/climada_python/blob/e41222210bfb99e92829c0a86f1db2f31355abf5/climada/engine/impact.py#L1246

Because "event_name" was always specified as a list of strings, I wrote code that only reads it as such. We could adapt that, too.

chahank commented 9 months ago

Well, the fact is that we do not enforce type on the impact objects. So, either we need to change that or update the over-restrictive read method.

peanutfun commented 8 months ago

How to continue here? Do we want to read other values than strings or do we want a better error message if the values are not strings? What do we do in this case? Continue by setting "default" event names or only throw an error?

peanutfun commented 6 months ago

@chahank @sarah-hlsn

sarah-hlsn commented 6 months ago

Thanks, @peanutfun for the reminder. To be honest I don't think I understand enough about to implications of any of the options you guys mentioned to give an educated opinion on this. I'd be happy to make a PR for any option you think is best though!

peanutfun commented 4 months ago

Do we want to read other values than strings or do we want a better error message if the values are not strings? What do we do in this case? Continue by setting "default" event names or only throw an error?

@chahank This does not seem to be a pressing issue so I would like us to take a pragmatic decision here and resolve it.

chahank commented 4 months ago

Agreed!

peanutfun commented 2 months ago

894 proposes a solution that reads other data types, but issues a warning that event_name cannot be decoded

peanutfun commented 1 month ago

Decision from discussion in https://github.com/CLIMADA-project/climada_python/pull/894:

emanuel-schmid commented 1 month ago

solved by #894