-
Notifications
You must be signed in to change notification settings - Fork 2
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
Flash minor changes (Merge to #469) #479
Conversation
Pull Request Test Coverage Report for Build 10064325275Details
💛 - Coveralls |
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.
Looks all good to me, a few suggestions in the comments.
@@ -220,15 +220,16 @@ def get_elapsed_time(self, fids: Sequence[int] = None, **kwds) -> float | list[f | |||
raise KeyError( | |||
"File statistics missing. Use 'read_dataframe' first.", | |||
) from exc | |||
time_stamp_alias = self._config["dataframe"].get("time_stamp_alias", "timeStamp") |
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.
`We should add this to the default config. It is also required e.g. in the normalization histogram function for timestamps
try: | ||
dtypes[channel] = channels_dict["dldAux"][channel].get("dtype") | ||
except KeyError: | ||
dtypes[channel] = None |
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.
Put as default float32 here?
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.
I think it's a bit risky if the data is coerced to 32 without users knowledge. I am not sure if a warning is provided by pandas if the data is float64.
Some minor refactoring. Should be merged to fix-459 and then to V1. Mainly: