some updates to module_stft , module_resample, random_utils and io.download #96
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #96 +/- ##
==========================================
+ Coverage 65.52% 65.64% +0.11%
==========================================
Files 81 81
Lines 5494 5504 +10
==========================================
+ Hits 3600 3613 +13
+ Misses 1894 1891 -3 ☔ View full report in Codecov by Sentry. |
paderbox/utils/random_utils.py
Outdated
|
|
||
| @dataclasses.dataclass | ||
| class Choice(_Sampler): | ||
| events: int = 1 |
There was a problem hiding this comment.
Choice and choice should have the same default for events.
While other distributions have some kind of natural defaults, I would say, we shouldn't set a default for events.
paderbox/io/download.py
Outdated
| target_dir = local_file.parent | ||
| else: | ||
| target_dir = Path(target_dir) | ||
| if not target_dir.exists(): |
There was a problem hiding this comment.
Remove this if, just call target_dir.mkdir(parents=True, exist_ok=True)
| raise FileExistsError(target_file) | ||
| tar.members = [] | ||
| os.remove(local_file) | ||
|
|
There was a problem hiding this comment.
Is there a reason, that you didn't define an else path with a raise? If not, could you raise a warning, when there is an unsupported suffix?
paderbox/io/download.py
Outdated
| for _ in ex.map( | ||
| download_file, | ||
| file_list, | ||
| [target_dir / Path(f).name.split('?')[0] for f in file_list], |
There was a problem hiding this comment.
Could you add a comment, why only name is used and why everything behind the ? is ignored?
Alternatively, a doctest could also help to understand the motivation.
paderbox/transform/module_stft.py
Outdated
|
|
||
| >>> [sample_index_to_stft_frame_index(i, 8, 1, fading=None) for i in range(12)] | ||
| [0, 0, 0, 0, 1, 2, 3, 4, 5, 6, 7, 8] | ||
| >>> sample_index_to_stft_frame_index(np.arange(12), 8, 1, fading=None) |
There was a problem hiding this comment.
Add a dtype for arange. The Windows test fails, because the default is int32 on windows.
| local_file: | ||
| exist_ok: | ||
| extract: | ||
| progress_par: |
|
|
||
| @dataclasses.dataclass | ||
| class Choice(_Sampler): | ||
| events: int = None |
There was a problem hiding this comment.
Why not simply events: int? (without a default value)
Then the `post_init``wouldn't be nessesary.
boeddeker
left a comment
boeddeker
left a comment
There was a problem hiding this comment.
When removing the default for events, LGTM.
4 participants
No description provided.