Improve verbose information output from cf.aggregate #232
Conversation
There was a problem hiding this comment.
The updated messages are clearer than the original ones in my eyes, excellent.
Though I could hack some fields to aggregate to run into the messages that have been changed here, to test, I didn't seem to hit them by ramping up the verbosity "to 11" (i.e. max debug/-1) on the aggregate test, so that suggests we can improve test coverage in future by including cases corresponding to attempted aggregation leading to these messages e.g. of fields where some have identical 1D coordinates, etc.
Also, during reviewing I noticed a few formatting errors in the higher-verbosity logging messages (probably mistakenly introduced by me during the original conversion from print statements), so I will put up a PR shortly in soon to fix those. I might also check all tests under debug verbosity to pick up on any obvious further log message issues towards that PR.
|
Thanks, Sadie. I'll add try and a unit test for these messages to this PR. Good idea to have a separate PR for the other message you have spotted. (Can we make |
It could be a nice easter egg! Though I think we planned to have an even higher logging level reserved for parallelisation-logic? In which case that would be the maximum and should be the 11... |
|
Hi @sadielbartholomew - may I merge this one, now? I think improving the test coverage as you suggest is one for another issue. |
There are a few conflicts to resolve, but once that is done, please go ahead and merge, thanks @davidhassell (generally, for future, if I give a formal 'approved' review I am happy for the PR to be merged unless there is something outstanding stated in the review comment, and I agree test coverage can be improved separately).
I didn't link in the relevant commit, like I should have ideally, but I did do this a while back, so that is sorted too. |
|
Thanks, Sadie |
Fixes: #228