Skip to content

Conversation

@ebrahimebrahim
Copy link
Collaborator

@ebrahimebrahim ebrahimebrahim commented Jul 10, 2024

Eventually the inner workings of Database are likely to be replaced.
But for now these improvements help to support the work on OpenwaterHealth/SlicerOpenLIFU#4

Close #79

Todo

Notes

empty list of markers

In the example data, I had to change example_session.json in order for Session loading to work here.

In example_session.json we have

  "markers": {
    "id": [],
    "name": [],
    "color": [],
    "radius": [],
    "position": [],
    "dims": [],
    "units": []
  },

which looks like this is trying to be an empty list of markers. But the openlifu Session represents markers as a list of Points, so it should be:

  "markers": [],

to represent an empty list of markers.

volume data versus volume ID

For this to work, another change to example_session.json is to replace the key "volume" by "volume_id".

This is because I have adjusted the Session API here to allow a Session to track a volume ID rather than insisting on loading the actual volume data. This way, the user of the Session api can decide for themselves how they want to handle volume file I/O.

Besides, in the example data the example_session.json already provides a volume ID rather than the actual volume data.

@ebrahimebrahim ebrahimebrahim changed the title Raise exception in unimplemented db methods Improvements to Database Jul 10, 2024
@peterhollender
Copy link
Contributor

One of the reasons for attaching the Volume was that I really want the database i/o to live at the periphery of the code. The Database object hasn't been typically passed around to other methods, as a Session object was sort of "fully-hydrated". However, I think it could work in this instance, because I did later introduce the concept of a Scene, which is a spatially-oriented view of the data from a Session. It could be in the conversion of Session to Scene that the volume is loaded. In the Python code, Session is defined in the db module already, so it makes me a little more comfortable having that operation involve the Database object

@ebrahimebrahim
Copy link
Collaborator Author

One of the reasons for attaching the Volume was that I really want the database i/o to live at the periphery of the code. The Database object hasn't been typically passed around to other methods, as a Session object was sort of "fully-hydrated". However, I think it could work in this instance, because I did later introduce the concept of a Scene, which is a spatially-oriented view of the data from a Session. It could be in the conversion of Session to Scene that the volume is loaded. In the Python code, Session is defined in the db module already, so it makes me a little more comfortable having that operation involve the Database object

That makes sense. In a way, the internals of Session and Subject are part of that database i/o periphery.

Besides volumes, another reason for passing the Database along when constructing a Session is to allow for it to look up the Transducer. The json file defining a Session only has a reference to a transducer, and without the Database object the Session.from_file method wouldn't know how to find the actual transducer definition file.

@peterhollender
Copy link
Contributor

One of the reasons for attaching the Volume was that I really want the database i/o to live at the periphery of the code. The Database object hasn't been typically passed around to other methods, as a Session object was sort of "fully-hydrated". However, I think it could work in this instance, because I did later introduce the concept of a Scene, which is a spatially-oriented view of the data from a Session. It could be in the conversion of Session to Scene that the volume is loaded. In the Python code, Session is defined in the db module already, so it makes me a little more comfortable having that operation involve the Database object

That makes sense. In a way, the internals of Session and Subject are part of that database i/o periphery.

Besides volumes, another reason for passing the Database along when constructing a Session is to allow for it to look up the Transducer. The json file defining a Session only has a reference to a transducer, and without the Database object the Session.from_file method wouldn't know how to find the actual transducer definition file.

Yeah, that tracks. In the MATLAB version, there is no Session.from_file, as it relies on the Database.load_session to load the session data from disk and hydrate it with the transducer and volume.

Issue #57

Loading a session doesn't actually work right now, but this commit
just enables the previously disabled code and implements a `from_file`
Issue #57

Previously it was wrongly trying to build a Transducer using the
transducer dict inside the session, rather than looking up
the transducer by the transducer ID provided in that dict.

In order to achieve the lookup, the Session needs to know about the
Database. This results in a circular interdependency between Session
and Database. There are multiple solutions, but I went with just
accepting the interdependency since it seems natural here.
This works towards #57 as it is needed to make loading a Session
possible (a Session contains targets and markers which are Points)
This is work towards #57
When loading a Session, I think it makes sense to allow the volume
data to remain a "reference" to a file rather than load the actual
data into memory. This way the users of the Session api can decide
how they then want to handle volume I/O.
@ebrahimebrahim
Copy link
Collaborator Author

I added the example database folder as a resource to be used for unit testing. I removed any large files from there and shrank the example volume so this should not bloat the repo

Copy link
Contributor

@peterhollender peterhollender left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should strip back the verasonics and M2 stuff from the example repository
Edit: didn't add the comments, so this is a duplicate review

Copy link
Contributor

@peterhollender peterhollender left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should strip back the verasonics and M2 stuff from the example repository

Cleans up the example database that we added
to unit test the work of #81 towards #57
@ebrahimebrahim ebrahimebrahim merged commit 38b8deb into main Aug 15, 2024
ebrahimebrahim added a commit that referenced this pull request Aug 15, 2024
@ebrahimebrahim ebrahimebrahim deleted the issue_79 branch September 13, 2024 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unimplemented methods in database.py should raise an exception

3 participants