gnn_tracking icon indicating copy to clipboard operation
gnn_tracking copied to clipboard

Create tests for `utils/plotting.py`

Open klieret opened this issue 1 year ago • 6 comments

klieret avatar Apr 13 '23 21:04 klieret

Hey @klieret Can I work on this?

kingjuno avatar Apr 25 '23 03:04 kingjuno

Sure. Probably some things can be tested rather trivially with the test data in the repository.

klieret avatar Apr 25 '23 15:04 klieret

image @klieret Do you think these files should be renamed (or fix the code)?

Especially because of this line: https://github.com/gnn-tracking/gnn_tracking/blob/75cc6020e5db33064d82b2330d15783aa72cefc2/src/gnn_tracking/utils/plotting.py#L123

  1. This already assumes that the prefix is event (but here it is test_event).
  2. In the example notebook, we use this event{evtid:09}-cells.csv.gz to set event_id to be a 9-letter string. But in this line (event0000{evtid}), the prefix won't be a 9-lettered string if evtid is less than 10000.

I can work with test data by renaming them, but by any chance does this needs to be fixed?

kingjuno avatar Apr 26 '23 05:04 kingjuno

Hi! Thanks, you're absolutely right; that's some bad stuff right there.

I'd say we do the following:

  1. Renaming the test files to start with event (like the original data)
  2. Changing event0000{evtid} also to event{evtid:09}

Thank you for taking such close a look!

klieret avatar Apr 26 '23 20:04 klieret

I am wondering whether there is a need for these two functions plot_3d, plot_rz (not part of any class; independent functions) as I don't see them used anywhere in the codebase.

kingjuno avatar May 02 '23 17:05 kingjuno

A lot of the plotting functions are used for analysis, which is done in separate notebooks. However, these two functions are part of the legacy codebase and they look somewhat less fleshed out than the rest.

I'd say: Let's keep them, but no need to test them for now :)

klieret avatar May 05 '23 15:05 klieret