search

alan-turing-institute / DetectorChecker

Project to develop software to assess developing detector screen damage (Web App based on the original DetectorChecker package)
https://detectorchecker.azurewebsites.net
MIT License
0 stars 1 forks source link

Joss Review #111

Closed janfreyberg closed 3 years ago

janfreyberg commented 4 years ago

Review issue: https://github.com/openjournals/joss-reviews/issues/2474

Test completeness

I noticed that some tests check for existence of JPG output and some don't (e.g. here). It would be good to ensure this is tested everywhere a file gets deleted.

Documentation

There are some functions where I feel I don't understand the underlying algorithm / functionality from the documentation:

  1. find_clumps - no description is given of how a clump is identified. Is this done simply by "continuous" unbroken sets of damaged pixels? I wonder if this could be included.
  2. In general, this applies to a few different functions - when calling help() on a function, I would prefer finding out the API and the conceptual functionality.
  3. The vignette in general is a fantastic resource and easy to follow. However, for plot_pixels_kfg I feel I missed a section that explains K/F/G plots. This may be my relative ignorance, but I would appreciate a reference or explanation of how these plots are used. You describe this very well in the paper, would it be possible to copy small sections of the text to the vignette?
  4. The documentation primarily refers to detector object attributes as slots and demonstrates accessing them using integer indexing. I think it would be worth changing this to name access, as it's self-documenting.

Code improvements

I note that the other review already mentions S3 objects and how detector objects are currently lists. It would be a nice improvement to make these actual S3 objects. This would allow for code improvements that would match up with how R users expect APIs to work. For example, this from the vignette:

cat(detector_summary(detector_pilatus))

could turn into this:

summary(detector_pilatus)

by the simple implementation of a summary method for detectors like so:

summary.Detector <- function(detector) cat(detector_summary(detector))

If Detector was an S3 object. I think this aligns better with R users, as calling summary and other base functions on objects is commonplace. The same could be done with plot_detector -> plot.Detector

This is a nit-pick and obviously involves considerable development for usability rather than functionality, though.

janfreyberg commented 4 years ago

The paper is great, the only thing missing in my view is a clearer summary of the field - what options do people currently have? If there is no other open-source package or no other package that implements this workflow from start to finish, it would be worth saying so.