-
Notifications
You must be signed in to change notification settings - Fork 0
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
Fix various bugs #336
Fix various bugs #336
Conversation
Co-authored-by: Jake Wagoner <[email protected]>
2b467be
to
9513218
Compare
Co-authored-by: Jake Wagoner <[email protected]>
…de if there is a subject to match
…e associated with the same particles object
af4577e
to
279d235
Compare
Rebased to exclude commit with |
…ks-cloud into fix-multi-domain
Co-authored-by: Jake Wagoner <[email protected]>
…ks-cloud into fix-multi-domain
…r and more reliable results)
49362c6
to
6e6aace
Compare
As of my recent pair-programming with @JakeWags, it appears that this PR fixes any multi-domain use cases. We confirmed the whole workflow (Groom, Optimize, Analyze) works on both single-domain and multi-domain test datasets. We can start the review process, but I would like to wait on merging this for the moment. Jake will soon be receiving a six-domain dataset from a collaborator, and it would be a good idea to test with that data before merging. So far, our only multi-domain datasets have had just two domains. |
I downloaded https://www.shapeworks-cloud.org/#/dataset/151/project/194 and uploaded it to my local instance then regroomed and reoptimized it. While playing with it, sometimes I don't see all of the domains: |
@manthey Is this happening after running through every operation, when using the "pre-cached" files, or both? |
After uploading the project, I reran groom and optimize with the default values. I saw the issue while playing with the results. If I do a hard reload of project, I can see the issue if all I do is turn on and off the original, groomed, and reconstructed layers. I can get it consistently to have an issue by just loading from default, picking groomed, picking reconstructed (then I think I see an issue), turning off original and turning off reconstructed (now I have only one domain showing). I see this in both Chrome and Firefox. |
I believe I found the cause: When we render shapes (not including particles), we send in shape data to the rendering process as a 2D array. For each domain, there is a list of overlapping shapes. When we cache these shapes, we use a unique label, which is supposed to include the index of the domain. We had been getting this index from
Because of this, our cached labels looked like this:
Going in this order of steps, at the third step, the shape stored for The solution is simple: don't flatten the 2D array. We only care about the index of the domain, not the index of the layer in the overlapping shapes. The layer is already reflected in the cache label. I've made this change in ee31dd3. |
I did find another issue while making the above fix. When viewing the Analysis tab for this dataset, it looks like the two domains are drawn on top of each other. I was able to confirm that these are two separate sets of shape and particle files. Perhaps this is the output of the analysis itself? I'm not aware that we do any centering / repositioning. We may just have to ask Alan. @JakeWags and I had noticed the same thing happening on the Hip Multiple Domain dataset. |
The analysis objects all being at the origin is due to no reference alignment being used to compute distance transforms. Alan mentioned potential options for individual domain reference, global reference, and Multi-Level PCA. These would likely be options set before the analysis is run (in optimization) or after the analysis has been generated (see std. dev. range). |
SCIInstitute/ShapeWorks#2154 will fix this issue. |
Ok, I agree that we should cut off the scope of this PR. I've made an issue for this bug so we can address it in a future PR: #347 |
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.
I say let's merge this and follow up with any remaining issues as other PRs.
This PR is essentially a bug hunt; it began with looking at multi-domain project use cases for unexpected behavior. Most of these fixes are related to multi-domain cases, but a few are miscellaneous bugs.
This PR resolves #330.
Results
The following bugs were identified and addressed in this PR:
Multi-domain
swproj
file. One use case project had keys like the following: "shape_1", "shape_2", "alignment_1", "alignment_2", "alignment_global", etc. Only two domain shapes exist for the subject, but a new anatomy was assumed, and this resulted in a third anatomy option "global" in the client, which contained no shapes. This was fixed by raising a warning (and not creating a new anatomy type) when a new suffix is encountered on any non-shape prefixes. Shape prefixes include "shape", "mesh", "segmentation", "contour", and "image".analysisFileShown
->analysisFilesShown
currentAnalysisFileParticles
->currentAnalysisParticlesFiles
meanAnalysisFileParticles
->meanAnalysisParticlesFiles
This fixed all particle comparisons for showing the difference from the mean shape and showing whether particles are good/bad. The attached videos demonstrate the fixed behavior.
shapeworks_multidomain_diff_from_mean.mp4
shapeworks_multidomain_good_bad_particles.mp4
I haven't been able to test the new store var structures with this use case because I don't have a dataset that has groups of multi domain subjects.With help from @JakeWags, the underlying structures for Group Comparisons were modified to support multi-domain cases as well.Miscellaneous
SET_NULL
behavior instead of theCASCADE
behavior.