Add import statements

[hackermd]

This will facilitate importing and using the package:

import highdicom as hd

seg = hd.seg.Segmentation(...)

[hackermd]

@CPBridge please note that I had to move two classes from highdicom.sr.coding into highdicom.coding_schemes, because I ran into circular import issues. I don't think these classes are widely used.

[CPBridge]

Generally in favour of this but I think there a few important things to think through:

1. Structure I notice you have chosen to import everything from files in the root of the repo (io, spatial, content) such that it appears at the top level of highdicom. This seems a little odd to me, since it ends up with what I consider to be more peripheral classes such as ImageToReferenceTransformer at the root of the module but more central classes such as Segmentation within a submodule. I propose keeping everything in spatial, io, uid, content within those modules so that every class exists one level below the main package (e.g. hd.spatial.ImageToReferenceTransformer and hd.seg.Segmentation) and it's essentially an implementation detail whether these submodules are implemented in a single file or spread across multiple files in a directory.
2. Documentation If we want this new scheme to be the "standard" way of using highdicom, I think we should make sure that the documentation is consistent with this in this PR. An easy fix is to the examples in the usage file. However other things becomes much more complex. Every time a class in mentioned in a docstring, e.g. as the type of a parameter, should we change this to the "new" canonical path? This might be quite tedious. Furthermore, it appears that currently your change has created errors in the documentation building process due to ambiguous cross-references.

[CPBridge]

@hackermd what about my two points in the overall comment above (structure and docs)?

[hackermd]

@hackermd what about my two points in the overall comment above (structure and docs)?

Sorry, I forgot to address those comments.

I generally agree with your structure proposal. However, there are few edge cases and additional considerations that I'd like to discuss. First, I would suggest exposing highdicom.utils and highdicom.content at the root level, so that it matches the behaviour of the subpackages (e.g., highdicom.sr.utils and highdicom.sr.content), where the content of these modules is exposed at the package level as well. Second, I like the fact that subpackages (namespaces) are organized by modality. In this regard, seg and sr are quite different from spatial and color. Organizing the implementations (functions, classes) that way is meaningful, but I am not sure whether we would need to enforce the namespaces. Note that one can still address them using the namespace, but one can also omit it (e.g., hd.spatial.ImageToReferenceTransformer and hd.ImageToReferenceTransformer currently both work). I think that's handy. What do you think?

Regarding documentation, I agree that it would be nice to use import highdicom as hd consistently and we should update the examples accordingly. However, I don't consider this critical at this point, since the docs will not be invalidated, i.e., one can continue to import the classes and functions directly. In principle, there is nothing wrong with that. Let me know whether you would want me to update before merging or whether we want to update for the next release (we may also want to factor examples out into separate *.py files that we can easily execute as part of the tests or run them via Sphinx). Thoughts? Preferences?

[CPBridge]

I think I agree about content remaining at the root. I personally would probably leave utils under hd.utils rather than at the root. After your message I'm not sure what you're proposing about spatial, color etc. I think using these top-level namespaces to group functionality is nice in the API as well as the implementation personally (although I agree that the enum, sop, content, utils, etc within the modalities currently takes it too far). This is similar to many other packages such as skimage and scipy

Regarding docs, sure If you prefer to do it later we'll do that. I was just thinking lower chance of forgetting if we do it now! Definitely think factoring out the examples to allow them to be tested is a good idea - nothing's worse than having the examples throw errors. The disadvantage is that there can be no more dcmread('example.dcm') with imaginary files sort of thing. But I think we can replace this with examples from the test data without making the example less clear

[CPBridge]

Also please remember to address the warnings in the documentation build process

[hackermd]

@CPBridge take a look at 3e1c246. I introduced a namespace for spatial, io, color, frame and utils at the root level of the package. The symbols in these modules are now accessible as follows

import highdicom as hd

transformer = hd.spatial.ImageToReferenceTransformer(...)

with hd.io.ImageFileReader(...) as reader:
    ...

For consistency, I also kept the utils namespace in the subpackages. For example

import highdicom as hd

matched_items = hd.sr.utils.find_content_items(...)

Is that what you had in mind?

[hackermd]

@CPBridge I also updated the usage section of the documentation accordingly. I don't make use of the Sphinx source code run feature. This will require a little bit more thought and effort. For example, we will need to select appropriate test data. I would like to address that with #69, which will require writing additional docs anyways.

I looked into the warning messages

lib/python3.9/site-packages/highdicom/seg/content.py:docstring of highdicom.seg.content.DimensionIndexSequence.get_index_values:: WARNING: more than one target found for cross-ref
erence 'PlanePositionSequence': highdicom.PlanePositionSequence, highdicom.content.PlanePositionSequence

I am not sure this is a problem. There are now two targets available and both of them are valid in my opinion. What do you suggest? Should we update the docstrings to reflect the namespace of the public API (i.e., use highdicom.PlanePositionSequence instead of highdicom.content.PlanePositionSequence)?

[CPBridge]

I introduced a namespace for spatial, io, color, frame and utils at the root level of the package.

I think that's a bit nicer. How do you feel about it?

For consistency, I also kept the utils namespace in the subpackages.

I don't personally think the inconsistency of having highdicom.utils as a separate module but importing the objects in highdicom.seg.utils into highdicom.seg (and same for sr) matters since it will be completely hidden from the users. So I would personally be tempted to include the contents of highdicom.seg.utils and highdicom.sr.utils into highdicom.seg and highdicom.sr respectively. What do you think? (Note that if you do this you will have to update package.rst and all docstrings referring to objects in seg.utils and sr.utils modules in the same manner as #72, this will make more sense to you as you read on...)

I am not sure this is a problem. There are now two targets available and both of them are valid in my opinion. What do you suggest?

I agree that the cause of the warning/error is not particularly important, but I still think we should aim to have the docs build with 0 warnings. That way it becomes much easier to check the build process and catch warnings that are actually important. I have fixed this issue in #72

Should we update the docstrings to reflect the namespace of the public API (i.e., use highdicom.PlanePositionSequence instead of highdicom.content.PlanePositionSequence)?

I spent a while playing around with this and I think we don't actually have a choice here. We have to use highdicom.PlanePositionSequence. In your last commit on this PR, building the docs resulted in two copies of the documentation for all the "duplicated" names (e.g. one entry for highdicom.PlanePositionSequence and another highdicom.content.PlanePositionSequence), which gives highly confusing docs. The only way to fix this is to remove the relevant items from the package.rst file so that the lower level copies are not included in the docs. This means that accordingly all the docstrings need to the use the higher level copy in order to have sphinx create the correct hyperlink for them. Gripped by regex mania I have fixed this in #72 and set it up to merge into the branch for this PR.

Also, why is the test on this branch failing due to some missing file?

[hackermd]

Also, why is the test on this branch failing due to some missing file?

Fixed with c7fd54b

[hackermd]

I introduced a namespace for spatial, io, color, frame and utils at the root level of the package.

I think that's a bit nicer. How do you feel about it?

I don't have a strong opinion here. From a usage perspective, I like giving each modality a dedicated namespace and keeping everything else under the root namespace. However, having separate namespaces for groups of related symbols may be relevant from a documentation perspective, since it would allow for a more intuitive structure and inclusion of additional descriptions (module docstrings) into the API doc.

[hackermd]

I don't personally think the inconsistency of having highdicom.utils as a separate module but importing the objects in highdicom.seg.utils into highdicom.seg (and same for sr) matters since it will be completely hidden from the users. So I would personally be tempted to include the contents of highdicom.seg.utils and highdicom.sr.utils into highdicom.seg and highdicom.sr respectively. What do you think? (Note that if you do this you will have to update package.rst and all docstrings referring to objects in seg.utils and sr.utils modules in the same manner as #72, this will make more sense to you as you read on...)

I see advantages of keeping utils separate. The content of the other modules (sop, content, etc.) generally contain symbols that represent DICOM attributes, functional groups, information object definitions, templates, etc. The functions in utils may not directly map to any of these standard concepts (recent discussion about grouping of single-frame image instances). Grouping them under a namespace is thus not a bad idea in my opinion.

[CPBridge]

Ok I think we're good to go. Regarding spatial, io etc I leave it as an executive decision to you, while leaning towards the way things are currently.