Adding READMEs to all directories #584

skeating · 2024-12-24T10:30:27Z

Description

Adds a README to all directories to facilitate easier navigation of the tree
Also adds comments where existing README did not make sense/was confusing
Corrected a few areas that were slightly confusing to read
Note changes to files other than README are from merging the main branch back into this one

Type of change

Please delete options accordingly to the description.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Suggested Checklist

I have performed a self-review of my own code.
I have made corresponding changes to the documentation.
My changes generate no new warnings.
I have commented my code, particularly in hard-to-understand areas.
I have passed on my local host device. (see further details at the CONTRIBUTING document)
Make sure your branch is up-to-date with main branch. See CONTRIBUTING for a general example to syncronise your branch with the main branch.
I have requested review to this PR.
I have addressed and marked as resolved all the review comments in my PR.
Finally, I have selected squash and merge

…s-pub

codecov · 2024-12-24T10:31:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.61%. Comparing base (aa702e3) to head (9fee555).
Report is 26 commits behind head on sk/joss-publication.

Additional details and impacted files

@@                 Coverage Diff                  @@
##           sk/joss-publication     #584   +/-   ##
====================================================
  Coverage                87.61%   87.61%           
====================================================
  Files                       76       76           
  Lines                     3505     3505           
====================================================
  Hits                      3071     3071           
  Misses                     434      434

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

skeating · 2025-01-13T10:01:20Z

Note: I have only made changes to README files, but I have imported changing from the main branch so a lot of files appear to have changed

stefpiatek · 2025-01-28T09:26:11Z

Would you mind if I rewrote this branch history just so that we don't get the changes coming up in the review?

tomaroberts

Hi @skeating – thanks for this. Various comments scattered throughout the review – some of them are just general thoughts. I do think the front page README needs an overhaul as a priority, and as I was reviewing the PR I was thinking the documentation should be in something like ReadTheDocs, but obviously that's a fair amount of work.

A couple of questions:

Is there a requirement from JOSS to have a README in every directory?
How did you generate all of the README files and the HTML and Markdown at the bottom of the files?
Do future developers need to update the READMEs manually according to any changes they make?

tomaroberts · 2025-03-04T15:59:07Z

README.md

Is the intention for the JOSS paper to publicise PIXL beyond UCL/UCLH?

Assuming the answer to above = yes, I think the root README needs a general overhaul because currently I think it gets too technical, too fast. For instance, the line 12 talks about UCLH technicalities, but this is irrelevant for someone approaching the repo outside of our institution or reading in conjunction with the journal paper.

I think the README should have more explanatory text and a simple diagram or image which conveys the key components within PIXL which are most useful to the community.

But – propose we do that in a separate PR.

tomaroberts · 2025-03-04T16:20:44Z

hasher/README.md

@@ -58,9 +58,40 @@ See the [Azure Key vault setup](../docs/setup/azure-keyvault.md) documentation f

 Save the credentials in `.secrets.env` and a LastPass `Hasher API <environment> secrets` note.

+SK QUESTION: is the reference to Last Pass something that is specific to us or is it a dependency somebody else would need. actually I assume the whole Azure thing is rather how we have chosen to do that rather than a necessity for somebody i.e. they might use a different system for storing their hashes


@stefpiatek – see "SK QUESTION" text.

tomaroberts · 2025-03-04T16:24:03Z

orthanc/orthanc-anon/README.md

@@ -1,7 +1,7 @@
 # Orthanc Anon

 _The Orthanc instance responsible for anonymising DICOM data from PACS/VNA and forwarding the images


Stray "_" character.

Suggested change

_The Orthanc instance responsible for anonymising DICOM data from PACS/VNA and forwarding the images

Orthanc Anon is responsible for anonymising DICOM data from PACS/VNA and forwarding the images

tomaroberts · 2025-03-04T16:28:32Z

orthanc/orthanc-anon/README.md

+- The Docker image is based on `orthancteam/orthanc`. <--- This is unclear where is this?
+- Configuration is driven through customised JSON config. files stored in the [orthanc-anon/config](./config/)


Suggested change

- The Docker image is based on `orthancteam/orthanc`. <--- This is unclear where is this?

- Configuration is driven through customised JSON config. files stored in the [orthanc-anon/config](./config/)

- Orthanc Anon is based on the Docker image: [orthancteam/orthanc](https://hub.docker.com/r/orthancteam/orthanc).

- Orthanc Anon is configured via a customised JSON config files stored in the [orthanc-anon/config](./config/)

tomaroberts · 2025-03-04T16:39:57Z

orthanc/orthanc-raw/README.md

@@ -22,7 +22,7 @@ available shortly when the service is started).

 ### Configuration

- The Docker image is a deployment of `orthancteam/orthanc` with some extra configuration
+- The Docker image is a deployment of `orthancteam/orthanc` with some extra configuration <--- is orthancteam/orthanc supposed to point to somewhere in the tree


Suggested change

- The Docker image is a deployment of `orthancteam/orthanc` with some extra configuration <--- is orthancteam/orthanc supposed to point to somewhere in the tree

- The Orthanc Raw instance is a deployment of the [orthancteam/orthanc](https://hub.docker.com/r/orthancteam/orthanc) container with the following additional configuration:

tomaroberts · 2025-03-04T17:22:24Z

pixl_export/README.md

@@ -37,8 +37,36 @@ pytest

 ## Usage

-Usage should be from the CLI driver, which calls the HTTP endpoints.
+Usage should be from the [CLI driver](../cli/README.md), which calls the HTTP endpoints.


I think this assumes a lot of understanding of the CLI and PIXL in general.

Again, if there is a desire for the different modules in PIXL to be used independently, then it would be good for the pixl_export documentation to explain how to use it.

For now, I think it would be good to put a few lines in this Usage section which demonstrate it woulds on the command line.

tomaroberts · 2025-03-04T17:24:29Z

pixl_export/README.md


 ## Notes

- The height/weight/GCS value is extracted only within a 24 h time window
+- The height/weight/GCS value is extracted only within a 24 h time window  <SK: I assume this refers to what can come out of OMOP   >


Agree – this either needs a fuller explanation or deleting.

tomaroberts · 2025-03-04T17:26:27Z

pixl_imaging/README.md

@@ -20,14 +20,14 @@ If the study has be identified in VNA or PACS, a query to `orthanc-raw` is made
 exists locally. If it does exist locally, a check is made to ensure all instances exist locally and any missing
 instances are retrieved. If the study does not exist locally, the entire study is retrieved from the archive.

-Once the study and all its instances are in `orthanc-raw`, the study is sent to `orthanc-anon` via a C-STORE
+Once the study and all its instances are in `orthanc-raw`, the study is sent to `orthanc-anon` via a C-STORE <SK: Please explain what C-STORE operation>


Suggest specifically calling it a "DICOM C-STORE" operation and linking to some documentation. Could link to the official NEMA DICOM docs, but they are a bit impenetrable, hence link I chose.

Suggested change

Once the study and all its instances are in `orthanc-raw`, the study is sent to `orthanc-anon` via a C-STORE <SK: Please explain what C-STORE operation>

Once the study and all its instances are in `orthanc-raw`, the study is sent to `orthanc-anon` via a [DICOM C-STORE operation](https://www.medicalconnections.co.uk/kb/Basic-DICOM-Operations).

tomaroberts · 2025-03-04T17:32:28Z

pixl_imaging/alembic/README.md

A general observation, exemplified by this particular README – the PIXL documentation has multiple 'voices' from where multiple developers have added to the docs where the language/tense etc. is inconsistent.

Here, the README begins in second person "We've followed...". Not something for this PR, but it would be good to make the language more consistent throughout. I think it's beneficial for interpretation.

tomaroberts · 2025-03-04T17:46:43Z

test/README.md

+We provide a [`pytest` plugin](../pytest-pixl/README.md) with shared functionality for the PIXL system
 and unit tests. This includes an `ftp_server` fixture to spin up a lightweight FTP server,
-to mock the FTP server used by the Data Safe Haven.
+to mock an FTP server being used to deposit the data.


General comment based on my experience of PIXL – I have found the testing quite confusing. We have (I think):

tests in each directory

the ability to run all tests at once (via pytest.ini) albeit this has caveats

the pytest-pixl plugin, which I don't really understand the motivation for why it's designed as a plugin

integration tests – these are made slightly complicated by .env shenanigans and subtle differences with the containers in the unit tests

What's my point... I'm just trying to illustrate that there is quite a bit going on with the testing and it's scattered through various parts of the documentation.

skeating added 4 commits November 27, 2024 15:55

update main read me

93dc6ba

update main read me (#529)

edbf6dc

Merge branch 'sk/joss-1' into sk/joss-pub

235ca58

Merge remote-tracking branch 'origin/sk/joss-publication' into sk/jos…

8d7faf6

…s-pub

skeating added 14 commits December 24, 2024 14:39

made read mes consistent

65c7b5f

Made the read mes more consistent

3dcc4fb

made read mes more consistent

a48ec93

making readmes consistent

7c777c9

making readmes consistent

4109db4

making readmes consistent

08b4ca5

making readmes consistent

474894e

Making readmes more consistent

b0360ea

Making readmes consistent

2babe4d

Merge remote-tracking branch 'origin/main' into sk/joss-pub

6bf6989

Making readmes consistent

415761f

Making readmes consistent

57f13b9

Making readmes consistent

e00eeae

Making readmes consistent

35c34b2

skeating requested a review from stefpiatek January 13, 2025 09:57

skeating marked this pull request as ready for review January 13, 2025 09:59

Merge remote-tracking branch 'origin/main' into sk/joss-pub

95be8f9

skeating requested a review from tomaroberts February 11, 2025 15:51

Merge branch 'main' into sk/joss-pub

9fee555

tomaroberts changed the base branch from sk/joss-publication to main March 4, 2025 10:23

tomaroberts changed the base branch from main to sk/joss-publication March 4, 2025 10:24

tomaroberts requested changes Mar 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding READMEs to all directories #584

Adding READMEs to all directories #584

skeating commented Dec 24, 2024 •

edited

Loading

codecov bot commented Dec 24, 2024 •

edited

Loading

skeating commented Jan 13, 2025

stefpiatek commented Jan 28, 2025

tomaroberts left a comment

tomaroberts Mar 4, 2025

tomaroberts Mar 4, 2025

tomaroberts Mar 4, 2025

tomaroberts Mar 4, 2025

tomaroberts Mar 4, 2025

tomaroberts Mar 4, 2025

tomaroberts Mar 4, 2025

tomaroberts Mar 4, 2025

tomaroberts Mar 4, 2025

tomaroberts Mar 4, 2025

		@@ -58,9 +58,40 @@ See the [Azure Key vault setup](../docs/setup/azure-keyvault.md) documentation f

		Save the credentials in `.secrets.env` and a LastPass `Hasher API <environment> secrets` note.

		SK QUESTION: is the reference to Last Pass something that is specific to us or is it a dependency somebody else would need. actually I assume the whole Azure thing is rather how we have chosen to do that rather than a necessity for somebody i.e. they might use a different system for storing their hashes

		@@ -1,7 +1,7 @@
		# Orthanc Anon

		_The Orthanc instance responsible for anonymising DICOM data from PACS/VNA and forwarding the images

	_The Orthanc instance responsible for anonymising DICOM data from PACS/VNA and forwarding the images
	Orthanc Anon is responsible for anonymising DICOM data from PACS/VNA and forwarding the images

		- The Docker image is based on `orthancteam/orthanc`. <--- This is unclear where is this?
		- Configuration is driven through customised JSON config. files stored in the [orthanc-anon/config](./config/)

	- The Docker image is a deployment of `orthancteam/orthanc` with some extra configuration <--- is orthancteam/orthanc supposed to point to somewhere in the tree
	- The Orthanc Raw instance is a deployment of the [orthancteam/orthanc](https://hub.docker.com/r/orthancteam/orthanc) container with the following additional configuration:

	Once the study and all its instances are in `orthanc-raw`, the study is sent to `orthanc-anon` via a C-STORE <SK: Please explain what C-STORE operation>
	Once the study and all its instances are in `orthanc-raw`, the study is sent to `orthanc-anon` via a [DICOM C-STORE operation](https://www.medicalconnections.co.uk/kb/Basic-DICOM-Operations).

Adding READMEs to all directories #584

Are you sure you want to change the base?

Adding READMEs to all directories #584

Conversation

skeating commented Dec 24, 2024 • edited Loading

Description

Type of change

Suggested Checklist

codecov bot commented Dec 24, 2024 • edited Loading

Codecov Report

skeating commented Jan 13, 2025

stefpiatek commented Jan 28, 2025

tomaroberts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skeating commented Dec 24, 2024 •

edited

Loading

codecov bot commented Dec 24, 2024 •

edited

Loading