Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
chahank opened this issue Nov 17, 2023 · 10 comments · Fixed by #894
Closed

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

chahank opened this issue Nov 17, 2023 · 10 comments · Fixed by #894
Assignees

Comments

@chahank
Copy link
Member

chahank commented Nov 17, 2023

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
Copy link
Member

This is not an issue with writing, but with reading:

kwargs["event_name"] = list(file["event_name"].asstr()[:])

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
Copy link
Member Author

chahank commented Nov 20, 2023

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
Copy link
Member

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
Copy link
Member

@chahank @sarah-hlsn

@sarah-hlsn
Copy link
Collaborator

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
Copy link
Member

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
Copy link
Member Author

chahank commented Apr 15, 2024

Agreed!

@peanutfun
Copy link
Member

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

@peanutfun
Copy link
Member

Decision from discussion in #894:

  • We only write an Impact object if event_name contains strings exclusively. If it does not, we throw an error instead of writing.
  • When reading, we expect strings stored in event_name. If they are not, we throw a warning and all str() on each element, potentially ending up with nonsensible values. But event_name is ensured to be strings after reading.

@emanuel-schmid
Copy link
Collaborator

solved by #894

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants