[TEP014] Added HDFWriter class + Unit Tests #744
Conversation
|
@wkerzendorf . Please review |
tardis/io/util.py
Outdated
| return data | ||
|
|
||
| @staticmethod | ||
| def convert_to_camel_case(s): |
There was a problem hiding this comment.
As I understand it, what we are doing here is converting to snake_case. Please rename the function accordingly.
tardis/io/util.py
Outdated
|
|
||
| @staticmethod | ||
| def convert_to_camel_case(s): | ||
| s1 = re.sub('(.)([A-Z][a-z]+)', r'\1_\2', s) |
There was a problem hiding this comment.
Can you please explain to me why we need two substitutions?
Additionally I don't understand why you added a [a-z]+ at the end of the capture group. This prevents AB from being converted to a_b, which I think we want?
There was a problem hiding this comment.
@yeganer I used this regex from this thread->Stackoverflow
tardis/io/util.py
Outdated
| @staticmethod | ||
| def convert_to_camel_case(s): | ||
| s1 = re.sub('(.)([A-Z][a-z]+)', r'\1_\2', s) | ||
| return re.sub('([a-z0-9])([A-Z])', r'\1_\2', s1).lower() |
There was a problem hiding this comment.
As mentioned above, I don't understand why we need this second substitution as the first seems to cover everything?
Maybe you can add some simple tests for some strings like CamelCase, ABC, ABcdefG, snake_Case?
|
I have used regex to convert from |
tardis/io/tests/test_HDFWriter.py
Outdated
|
|
||
|
|
||
| def test_snake_case(): | ||
| obj = MockHDF(None) |
There was a problem hiding this comment.
you don't need this line because convert_to_snake_case is a static method. You can simply write MockHDF.convert_to_snake_case(...)
tardis/io/tests/test_HDFWriter.py
Outdated
|
|
||
| arrays = [['L1', 'L1', 'L2', 'L2', 'L3', 'L3', 'L4', 'L4'], | ||
| ['one', 'two', 'one', 'two', 'one', 'two', 'one', 'two']] | ||
| tuples = list(zip(*arrays)) |
There was a problem hiding this comment.
I think this is a very complicated way to create these arrays. I'd suggest you use np.array(...) and then simply transpose it.
|
@wkerzendorf This PR consists of both |
vg3095
changed the title
No description provided.