A python library containing a reader and data class for vernier output#218
A python library containing a reader and data class for vernier output#218EdHone wants to merge 24 commits intoMetOffice:mainfrom
Conversation
Dataclass implementation for calipers
| cumul_time: list[float] | ||
| n_calls: list[int] | ||
| name: str | ||
|
|
There was a problem hiding this comment.
if we're already using numpy, then why are these list[float] rather than np.array?
There was a problem hiding this comment.
Numpy append creates a new array each time rather than using an dynamically allocated block of memory, so in short we use list[float] because it's a lot faster to build our dataset this way. We could allocate our data at the beginning but this was still more expensive and a bit clunky so I went down the route of using basic lists.
Performing the single allocation when doing something like np.mean(timers.data['timestep_alg'].total_time) is really cheap, but we do miss out on the nice functionality of being able to just perform timers.data['timestep_alg'].total_time.mean(). We could probably implement a solution that captures the best of both worlds in the future.
mo-marqh
left a comment
There was a problem hiding this comment.
coding standards; prefer newline at end of each file
Aggregator implementation
Fixed in commit 68a901 |
|
Open questions:
|
|
@mo-marqh - I think this is now ready for review and merge |
mo-marqh
left a comment
There was a problem hiding this comment.
I'd like
https://github.com/MetOffice/Vernier/blob/main/post-processing/post-process.py
removed as part of this change please, so that the commit is replacing that code with this (not having them side by side).
i'd also prefer to see a flat layout,
https://packaging.python.org/en/latest/discussions/src-layout-vs-flat-layout/
the lib/ directory seems superfluous.
prefer
post-processing/vernier
post-processing/tests
post-processing/tools
contents of the post-processing/example should move to post-processing/tools so they can easily be provided as CLI runnables
Additionally tests should include a test that runs the CLI tools (i didn't think this was tested here currently)
| @@ -0,0 +1,33 @@ | |||
| # ------------------------------------------------------------------------------ | |||
There was a problem hiding this comment.
#!/usr/bin/env python3
please
|
Thanks Mark - I believe I've addressed all your review comments and fixed a bit of non-deterministic behaviour when reading on multiple processes. I'll pass the PR back to you |
Description
This adds a python library with two classes which enable a user to read vernier output into a
VernierDataclass. This class has a simplefilterroutine which allows a user to extract the calipers that they are interested in, as well as a simple routine to print the averaged output to atimer.txt-like table.I've also added an example script which demonstrates the usage of these stuctures, along with some example vernier output data.
Linked issues
Closes # (issue)
Type of change
How has this been tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: