-
Notifications
You must be signed in to change notification settings - Fork 149
Explicitly convert event_name to strings in Impact.from_hdf5
#894
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
Conversation
|
why don't we just force event names to be strings? If event_name = [str(x) for x in file["event_name"][:]] |
|
@emanuel-schmid There was no decision on what to do in #807, so I came up with the least intrusive solution (from my perspective). I am fine with your proposal, but it would imply that an |
|
Yeah, right. Sorry for coming late to the discussion - but... |
|
@emanuel-schmid Thanks a lot for that perspective! This is exactly the kind of perspective I was hoping for in the discussion of #807. I would be fine with going the "string-only" route. However, there are some inconsistencies we then have to fix. In the tests for Impact, there are two instances where Impacts are instantiated with non-string event names:
|
|
@chahank What do you think? |
|
Thanks for the discussion @peanutfun and @emanuel-schmid : I here would also support your suggestion of the |
|
@peanutfun Yes, Sorry again. 😳 |
|
@emanuel-schmid @chahank When we go string-only, we still have to clarify: Do we (try to) convert any event name data to strings when writing, when reading, or both times? |
|
Suggestion: When writing we simply assume they're strings, when reading we convert if necessary. |
Why? We can also call |
|
Sure we can. But what would be the point? By then we supposedly have already been working daredevil on an "irregular object". We should have converted the non string event names to strings long ago. If we didn't, the conversion to strings just for the writing cannot save us as it doesn't make a difference anymore. Or does it? |
|
I think it's mainly a question of conventions. Currently, |
|
Hold on - when writing, the proper thing to do is fail if it is not a string, isn't it? Because if we call |
|
@emanuel-schmid Great catch! Then we
|
Try to convert to strings when reading.
event_name in Impact.from_hdf5event_name to strings in Impact.from_hdf5
chahank
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go.
Maybe we need an Issue to make sure that the event name is always a string in all hazard methods?
We would even need it for |
Changes proposed in this PR:
stron values if decoding strings fromevent_namefails, issue a warning in this case.event_nameis not strings.event_namewas checked.This PR fixes #807
PR Author Checklist
develop)PR Reviewer Checklist