feat: Tonality ECMA418-2 #208
Conversation
a-bouth
requested review from
anssmolla,
anscquesson,
anshlachamb and
ansaminard
as code owners
|
Important note (@ansaminard ) the outputs tonality over time and tone frequency over time might have slightly different time scales (same behaviour as in SAS), as a consequence :
|
github-actions
bot
added
documentation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #208 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 69 70 +1
Lines 3813 3878 +65
=========================================
+ Hits 3813 3878 +65 ☔ View full report in Codecov by Sentry. |
| time_scale_ft = self.get_output()[2].time_freq_support.time_frequencies.data | ||
|
|
||
| # Plot ECMA 418-2 parameters over time. | ||
| _, axes = plt.subplots(2, 1, sharex=False) |
There was a problem hiding this comment.
@a-bouth why not sharing X axis?
On the plot you share in the description of the PR, X axes are both vs time, but slightly not synchronized, which looks weird
There was a problem hiding this comment.
by the way, isn't the same time scale (get_time_scale()) shared by both curves ?
If yes: we can use the same, and share it among both plots
If no: why ? and in this case, we probably miss a time_scale getter for the second one
There was a problem hiding this comment.
@anshlachamb I think the question is for me rather than for Antoine
The answer is because both output does not have exactly the same time scale (same behaviour as in SAS)
Concerning the "why" : I have no precise answer. As for the getter I don't know if it's needed, I leave that question to @ansaminard
There was a problem hiding this comment.
"why": I don't know either, I planned to look into the Matlab code, but haven't the chance to do it yet. Anyway, whatever I find there, I doubt it'll have an immediate impact here, since @a-bouth says it's also the case in SAS
Added class
TonalityECMA418_2+ Associated tests