add esmf summary parser

[edoyango]

This add's a parser for esmf's text summary which can be parsed "flat" or hierarchically. We probably only need flat for now, but the latter might be useful at some point.

[micaeljtoliveira]

@edoyango I would suggest you activate the pre-commit hook in your development environment. It will likely save you some time.

[micaeljtoliveira]

@edoyango Thanks for the changes. All good from my side.

[manodeep]

@edoyango The linter check is failing - will you please fix that. I am looking through the rest of the code in the meantime

[manodeep]

I made some suggestions trying to reduce the hardcoding. This looks quite substantial and great overall.

The other bit would be that the code quality checks for the linter and coverage need to pass

[manodeep]

What happens if metric_keys contains extra keys? (Is that even possible for valid data?)

[edoyango]

metric_keys is the expected "metric" keys in both input_dict and correct_dict. The function will error if input_dict and correct_dict don't have all the keys in metric_keys. extra_keys would be the region keys.

[edoyango]

renamed extra_keys to region_keys so hopefully it's a bit clearer what they are

[minghangli-uni]

Sorry I haven’t looked into the code yet, but I’d suggest keeping only hierarchical, since some region names might appear in multiple locations (such as in Runphase or in Init). Otherwise it could lead to unexpected results.

[edoyango]

some region names might appear in multiple locations (such as in Runphase or in Init). Otherwise it could lead to unexpected results.

Are you talking about regions like [ensemble] RunPhase1, [ESM0001] RunPhase1 etc.? In the OM3 output I saw RunPhase1 many times, but they seemed to be prefixed uniquely. Are you saying that they could also have the same prefix too? Do you have an example handy?

[minghangli-uni]

For example, [ATM-TO-MED] RunPhase1 exists in both [ensemble] Init 1 and [ensemble] RunPhase1.

[micaeljtoliveira]

For example, [ATM-TO-MED] RunPhase1 exists in both [ensemble] Init 1 and [ensemble] RunPhase1.

I think that's fine. It's very common case when profiling a code that the same block of code is called from different places, with different data and, therefore, with very different timings. The hierarchical data gives you more information, which is good, but the flat one is still useful in many cases.

[edoyango]

True. Thanks for pointing out the issue.

@manodeep @micaeljtoliveira to either of you have a suggestion for how to handle this? I'm thinking for the flat case, we simply add a _N suffix for now. We could add the bracketed prefixes, but that would probably be too verbose since there's around 10 levels?

[micaeljtoliveira]

@manodeep @micaeljtoliveira to either of you have a suggestion for how to handle this? I'm thinking for the flat case, we simply add a _N suffix for now. We could add the bracketed prefixes, but that would probably be too verbose since there's around 10 levels?

As I wrote above, I think it's fine to aggregate calls to the same region but from different places in the call-tree together when working with the flat case. We loose some information, but what is left can still be useful.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (815072f) to head (0d6b41c).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main       #12   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9        10    +1     
  Lines          285       345   +60     
=========================================
+ Hits           285       345   +60     

☔ 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.

[edoyango]

some region names might appear in multiple locations (such as in Runphase or in Init). Otherwise it could lead to unexpected results.

@minghangli-uni @micaeljtoliveira @manodeep I've added some code to aggregate regions that occur more than one, if PETs and PEs are the same (errors if they're not). This assumes regions with identical names is referring to the same code. Can you all have a look before I merge?

[micaeljtoliveira]

@edoyango LGTM, just one very small thing to fix.

[minghangli-uni]

LGTM, thanks @edoyango