Add handling of data series and scaling data

[micaeljtoliveira]

No description provided.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (be8eb31) to head (460c24c).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main        #8   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         8    +1     
  Lines          188       211   +23     
=========================================
+ Hits           188       211   +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[micaeljtoliveira]

@manodeep This one should now be ready for review.

[micaeljtoliveira]

@manodeep I've tried to address all your comments. Please have a look and let me know if you're happy with the changes.

[manodeep]

@micaeljtoliveira tmax on this line is smaller than tmin - is that supposed to happen?

[manodeep]

Related - do we have any sanity checks on the values that tmin <= tavg <= tmax etc?

[micaeljtoliveira]

These numbers were originally added by Edward in the other repository and I don't think they were meant to be consistent, as they are only used for testing the parser.

I'm not sure we want to sanity check the numbers coming out of the logs, as that would be a lot of code and a lot of work. For now, let's just assume they are correct.

[manodeep]

That's fine to assume that the numbers are correct coming from the log files, but can we then swap the numbers in tmin[0] and tmax[0] in this mock profiling data?

[micaeljtoliveira]

Well, if it bothers you that much, yes, we can swap them. But note that in the context of these unit tests, the actual numbers are quite meaningless.

[micaeljtoliveira]

But then most of the other numbers are also problematic: tavg numbers for the "Total runtime" are clearly wrong and tavg for the other region in the 2 CPU case is greater than tmin and tmax...

[manodeep]

It probably shouldn't but it does actually bother me 😅

How about a compromise - can we add this an issue/feature request/test to make sure that the semantics of the parsed data are validated?

[micaeljtoliveira]

I've decided to change the data entirely. Looking at the code being tested, there was no point in having so many metrics. Instead, it seemed more important that the dimensions of the dataset (npucs and number of regions) were different. As said above, the actual numbers are not important, but I made sure tmax values were greater than tmin 😉

[manodeep]

The linked comment is empty but so I can get another review in. There are two comments that I have made inline as a "reply"

[manodeep]

Thanks for the updates - everything looks good. One minor comment - would it be possible to swap the tmin[0] and tmax[0] numbers in the profiling data? That way a future maintainer/reader does not get confused by the semantics of the fake data.

(I am being extra careful since this feature is likely to be re-used by other teams and potentially external folks.)

[manodeep]

That's fine to assume that the numbers are correct coming from the log files, but can we then swap the numbers in tmin[0] and tmax[0] in this mock profiling data?

[manodeep]

LGTM!

[micaeljtoliveira]

@manodeep Thanks for the review! Feel free to open an issue about the data validation for further discussions.