search

AustralianCancerDataNetwork / pydicer

PYthon Dicom Image ConvertER
https://australiancancerdatanetwork.github.io/pydicer/
Apache License 2.0
20 stars 5 forks source link

Patient logger class #123

Closed dalmouiee closed 1 year ago

dalmouiee commented 1 year ago

Hey @pchlap, I'd had a go at implementing what a patient summary logger may look like. I've implemented it for the Convert module, it can be extended to the downstream modules (visualise and dataset) which is probably where it'll show its greatest gains. What do you think of it and its extensibility? Thanks :)

dalmouiee commented 1 year ago

Hi @dalmouiee, sorry for the slowness in getting to this review.

This is looking really good. But it looks like this only logs when there is an error? Would we also be able to log successful operations? This would help us know for which patients we still have issues since we might run once and fail then correct the issue and run again and pass.

Another data point that would be useful here is to log the runtime. If we could have this data in our log it would help us in estimating the conversion time etc of future datasets.

Finally, it would also be good to log which piece of data the log actually refers to. So if there is an error we can easily find that object. So an additional column tracking the "hashed_uid" (or whatever we call it) would be useful. If we have the hashed_uid, then we could pull the path from the converted.csv log.

So I suppose the columns of the log could be:

* module

* hashed_uid

* success (boolean flag)

* error (blank if no error)

* start time

* end time

What do you think?

Great idea! That'll will definitely make it more informative and helpful for those edge case patients and subsequent dataset prep steps. I'll add those in and get your feedback on it :)

dalmouiee commented 1 year ago

Hi @pchlap, I've updated with your suggestions. I've tested this on the test dataset but keen to run this on ICCC breast data and see how it plays out. Let me know what you think :)

dalmouiee commented 1 year ago

Thanks @dalmouiee. Looks great. One minor comment I'd make is that some lines of code could probably be reduced by moving the start and end time calculation logic into the PatientLogger class. Like if we define the behavior as being on patientLogger init, start time is captured. Then an error is logged or the module logs its finish then the end time is determined in that function call. What do you think?

Up to you if you want to make that change now or not. But I'll leave my approval, happy to merge this through and for you to test it at ICCC. Longer term would be good to make sure this is tested in our testing suite but happy for that to be added at a later time (planning on a bit of a clean-up of the entire code base in the coming weeks where hopefully we can add tests which are missing across the library).

Great stuff though, thanks!

Awesome, I've made that change thanks for the suggestion. I'll go ahead and merge this now to test at ICCC next. Definitely keen to add this to the testsuite on that testdata!