SCIInstitute / ShapeWorks

ShapeWorks
http://sciinstitute.github.io/ShapeWorks/
Other
104 stars 32 forks source link

Spatiotemporal SSM - Disentangled Approach #2056

Closed nawazishkhan1-nk closed 1 year ago

nawazishkhan1-nk commented 1 year ago

[WIP] - Do Not Merge

Summary

This PR addresses issue #2055.

Changes Made

Testing Done

Known Issues

Screenshots

Screen Shot 2023-04-27 at 7 51 15 AM
akenmorris commented 1 year ago
  1. We need a small automated test case using this (e.g. OptimizeTests).

  2. It looks like the checkbox is visible and enabled (can be toggled) whether it's a multi-domain model or not. This may be confusing for novice users as we don't want to overwhelm them with options that aren't relevant to their data.

I think it's fine as a first step to just have only this optimization setting, but in the future we may want more of a data level option for indicating that these are time points rather than joint anatomies. This could enable other visualization modes (e.g. add a time point selector, or separate time points along the X axis). We could do this either with a flag that the user provides or by extending the input types (e.g. instead of shape_<name> column, we have shape_<name>_time_0, shape_<name>_time_1 and then when these are recognized, the spatiotemporal options become visible, or enabled. @sheryjoe, what do you think?

akenmorris commented 1 year ago

Can the checkbox be called "Spatiotemporal SSM"? I'm not sure the "disentangled" word adds much understanding.

nawazishkhan1-nk commented 1 year ago

Can the checkbox be called "Spatiotemporal SSM"? I'm not sure the "disentangled" word adds much understanding.

Agreed. I added "Disentangled" to distinguish it from the other Spatiotemporal SSM work using Polynomial Regression which we had published. Maybe, we can remove it in the meantime.

@sheryjoe Are we planning to integrate both methods for Spatiotemporal SSM in ShapeWorks Studion for users?

akenmorris commented 1 year ago

@nawazishkhan1-nk , do you have an update on the PR?

nawazishkhan1-nk commented 1 year ago

@nawazishkhan1-nk , do you have an update on the PR?

I think this PR is good to go considering the backend optimization. The only feature that has to be worked on is exposing a better way to input time points for each subject in the Data view of Studio.

akenmorris commented 1 year ago

Be advised that I had to convert the inverse covariance matrix from VNL to Eigen:

akenmorris commented 1 year ago

@nawazishkhan1-nk , I've fixed this branch for the changes in #2118.

Can you point me to a (hopefully small) example so that I confirm the result are unchanged (and to confirm future refactoring)?

akenmorris commented 1 year ago

I am merging this now so that I don't have to spend a bunch of time resolving conflicts as I make changes to these same files. I will create a separate issue for the automated test.