-
Notifications
You must be signed in to change notification settings - Fork 7
Improvements to Database
#81
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
Conversation
Database
|
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 |
That makes sense. In a way, the internals of 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 |
Yeah, that tracks. In the MATLAB version, there is no |
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.
|
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 |
There was a problem hiding this 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
peterhollender
left a comment
There was a problem hiding this 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
Eventually the inner workings of
Databaseare 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.jsonin order forSessionloading to work here.In
example_session.jsonwe havewhich looks like this is trying to be an empty list of markers. But the openlifu
Sessionrepresents markers as a list ofPoints, so it should be:to represent an empty list of markers.
volume data versus volume ID
For this to work, another change to
example_session.jsonis to replace the key"volume"by"volume_id".This is because I have adjusted the
SessionAPI here to allow aSessionto track a volume ID rather than insisting on loading the actual volume data. This way, the user of theSessionapi can decide for themselves how they want to handle volume file I/O.Besides, in the example data the
example_session.jsonalready provides a volume ID rather than the actual volume data.