Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decompose findscu to be more modular and have an easy-to-use library #352

Open
qarmin opened this issue Jun 2, 2023 · 2 comments
Open
Labels
A-lib Area: library C-findscu Crate: dicom-findscu help wanted

Comments

@qarmin
Copy link

qarmin commented Jun 2, 2023

Dicom, pynetdicom, dicom-rs I have been trying to understand and use them with varying degrees of success for 1 week, so as you can see my experience is limited, but I have noticed that it is quite difficult to use this library).

However, there are a few things that I found while trying to implement worklist(only find), so that for scheduled examinations from a given day I could send patient data(name, id, date of birth, etc.) to another application via e.g. Redis.

Apparently, I could try to parse the string output from findscu, but it would not look very good(additionally worklist not supported in this project).

  • There is no way to use imported code from findscu in external projects. I'm forced to copy almost the whole code from project and modify parts of it.
    E.g. functions find_req_command, parse_queries, term_to_element could go into some more general module so that they could be used without having to copy all that code.

  • The code in the run function in the findscu tool has about 200 lines of code and a similar implementation in python has with imports ~60 lines(code below) so it making much harder to understand entire app.
    Python code is more forgiving and ignores a mass of errors but is also much easier to use.
    E.g. the code that creates commands to send and sends them should be split into pieces and generalized.

let cmd = find_req_command(abstract_syntax, 1);
let mut cmd_data = Vec::with_capacity(128);
cmd.write_dataset_with_ts(&mut cmd_data, &entries::IMPLICIT_VR_LITTLE_ENDIAN.erased())
.whatever_context("Failed to write command")?;
let mut iod_data = Vec::with_capacity(128);
dcm_query
.write_dataset_with_ts(&mut iod_data, ts)
.whatever_context("failed to write identifier dataset")?;
let nbytes = cmd_data.len() + iod_data.len();
if verbose {
debug!("Sending query ({} B)...", nbytes);
}
let pdu = Pdu::PData {
data: vec![PDataValue {
presentation_context_id: pc_selected_id,
value_type: PDataValueType::Command,
is_last: true,
data: cmd_data,
}],
};
scu.send(&pdu).whatever_context("Could not send command")?;
let pdu = Pdu::PData {
data: vec![PDataValue {
presentation_context_id: pc_selected_id,
value_type: PDataValueType::Data,
is_last: true,
data: iod_data,
}],
};
scu.send(&pdu)
.whatever_context("Could not send C-Find request")?;

import sys
from builtins import *
from pydantic import BaseModel
from pynetdicom import (
    AE,
    BasicWorklistManagementPresentationContexts,
    QueryRetrievePresentationContexts,
)
from pynetdicom.apps.common import create_dataset
from pynetdicom.sop_class import ModalityWorklistInformationFind

from typing import Optional


class Temp(BaseModel):
    addr = '127.0.0.1'
    port = 4242
    keyword = [
        'PatientID',
        'PatientName',
        'PatientBirthDate',
        'PatientSex',
        'ScheduledProcedureStepSequence[0].ScheduledStationAETitle',
        'ScheduledProcedureStepSequence[0].ScheduledProcedureStepStartDate=19951015',
        'ScheduledProcedureStepSequence[0].ScheduledProcedureStepStartTime'

    ]
    file: Optional[str] = None


def main():
    args = Temp()

    identifier = create_dataset(args, None)
    ae = AE()

    ae.requested_contexts = QueryRetrievePresentationContexts + BasicWorklistManagementPresentationContexts

    query_model = ModalityWorklistInformationFind

    assoc = ae.associate(
        args.addr,
        args.port,
    )
    if assoc.is_established:
        responses = assoc.send_c_find(identifier, query_model)
        for (status, rsp_identifier) in responses:
            if status and status.Status in [0xFF00, 0xFF01]:
                print(status)
                print(rsp_identifier)
                print(f"RRRR    {rsp_identifier.PatientID}")

        # Release the association
        assoc.release()
    else:
        sys.exit(1)


if __name__ == "__main__":
    main()

@Enet4
Copy link
Owner

Enet4 commented Jun 2, 2023

Thank you for your well informed feedback, @qarmin! I can comment on the two points in particular, but the short version is that I not only agree that the library APIs can be extended with better abstractions for building high level DICOM software, it is also in my plans to have them in the project.

  • dicom-findscu currently only exists as a command line tool, but it definitely makes sense for it to also incorporate a library. dicom-dump went through the process of starting out only as a tool, and now it is also a library (that is even used in findscu!). One of the reasons for this is that such a library would depend on the constructs for building full-fleshed SCUs and SCPs, which are still in their infancy.
  • A good portion of the code bloat found in the tool is due to having to work with lower level abstractions. Creating a DICOM object representing the command needs to be done manually (including the command group length¹, which admittedly is an error-prone process) and encoded into a PDU, so that it can be sent through the client association. This appears to have been better streamlined in Pynetdicom, which already offers common presentation contexts by name, a send_c_find method, and a function to turn command-line arguments into data sets³.
    Not having to wait for the aforementioned abstractions for creating DICOM network application entities meant that the project could have a proof of concept sooner, and from which further work and design decisions could be made. With that said, a better direction for developers to embed SCUs in their own software should indeed be made moving forward.

This does not quite have to do with the lack of modularity in the project, but with missing pieces in these modules which need time and effort to develop. I have not had much of these two lately, and attracting contributors to close these tasks is one of the open challenges, but we will get there! I can try to split the feedback into separate issues in the meantime. See also the project roadmap.


Addendum: In DICOM-rs 0.6.0, a few things were introduced that make some of these things a bit easier:

¹ Constructing command objects is a bit easier using InMemDicomObject::command_from_element_iter: #365

² Constants to standard UIDs are now available in dicom-dictionary-std: #372

³ The operations API is already used in the latest version of dicom-findscu in order to let the user specify more complex queries (#326)

@jmlaka
Copy link
Contributor

jmlaka commented Jun 2, 2023 via email

@Enet4 Enet4 added A-lib Area: library C-findscu Crate: dicom-findscu labels Jun 3, 2023
@Enet4 Enet4 changed the title Make project more modular and easier to use Decompose findscu to be more modular and have an easy-to-use library Sep 2, 2023
@Enet4 Enet4 mentioned this issue Sep 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lib Area: library C-findscu Crate: dicom-findscu help wanted
Projects
None yet
Development

No branches or pull requests

3 participants