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

RE: issue 42 #49

Closed
wants to merge 6 commits into from
Closed

Conversation

JEHoctor
Copy link
Collaborator

This is a very superficial change to address #42. The benefits of vtkMRMLTableNode are not really seen here. (There is a reduction in LOC though.)

For example, the old system for saving the data is maintained. The data still actually lives in lists of special-purpose classes. (Each object represents a row. There is a class for rows of each table.) It would be better to make the table itself the authoritative repository of each kind of data.

What is the best way to save the contents of a vtkMRMLTableNode to a CSV file?

  • SaveWithScene is set to False

Should I create q3d.measurements.distance as an alias for q3dc.distance_table, and similarly for the other kinds of measurements?

Let's not merge this right away as we need to give Lucia et al. time to evaluate PRs #45 and #47.

Copy link
Contributor

@jcfr jcfr left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.

What is the best way to save the contents of a vtkMRMLTableNode to a CSV file?

For loading a table:
https://github.com/Slicer/Slicer/blob/9f124db1fcf68eceb779b1414b634f0cd4fc1c52/Base/Python/slicer/util.py#L640-L645

For saving a table:
https://github.com/Slicer/Slicer/blob/9f124db1fcf68eceb779b1414b634f0cd4fc1c52/Base/Python/slicer/util.py#L754-L762

For reference, reader is implemented here

Should I create q3d.measurements.distance as an alias for q3dc.distance_table, and similarly for the other kinds of measurements?

I am not sure to fully understand the question, that said here are some additional comments:

  • Consider adding qMRMLTableView to the UI files
  • Check that module still works after closing the scene

@bpaniagua
Copy link
Contributor

Hi @JEHoctor

Can you please work on amending this commit so to address the requests from @jcfr ?
Thank you!

Bea

@allemangD
Copy link
Contributor

Closing. I have made some more changes on my own fork, building off of James's progress here, and will merge those in a separate PR.

@allemangD allemangD closed this Mar 15, 2021
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.

4 participants