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

Used the absolute path to create the temporary label file in propseg #1487

Merged
merged 14 commits into from
Oct 14, 2017

Conversation

peristeri
Copy link
Contributor

Description of the Change

The filename for the temporary label file did not match with the one used when cleaning up.

Applicable Issues

Implements or Fixes #1483

@peristeri peristeri added bug category: fixes an error in the code card:TO_REVIEW priority:HIGH labels Oct 11, 2017
@peristeri peristeri added this to the Release v3.0.9 milestone Oct 11, 2017
@jcohenadad
Copy link
Member

jcohenadad commented Oct 13, 2017

tested-- still failing. Should we reinstall sct?
EDITED: yes, SCT needs to be reinstalled with pip install. @peristeri: please ALWAYS mention at the beginning of the PR if SCT needs to be reinstalled, and how to do it. Something like this:

For testing this PR, go to SCT folder and run:

source python/bin/activate
pip install -e .

@jcohenadad
Copy link
Member

Tested the viewer. There is no crash, however the functionality is broken. Example here:
https://www.dropbox.com/s/ekeqwlfsgj7ihxt/20171013_bugViewer.m4v?dl=0

@peristeri: This is HIGH priority. If you cannot fix it today, please come back to the previous version of the viewer we had prior to introducing the QT version.

@peristeri
Copy link
Contributor Author

peristeri commented Oct 13, 2017 via email

@jcohenadad
Copy link
Member

@peristeri: this not correct. -init-mask asks for three points while -init-centerline asks for any number of points along the centerline.

@peristeri
Copy link
Contributor Author

peristeri commented Oct 13, 2017 via email

Copy link
Contributor

@benjamindeleener benjamindeleener left a comment

Choose a reason for hiding this comment

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

Hi George, I tested the viewer. The centerline functionality works now. I saw a couple of things that should still be changed:

  1. The title on the right panel should be only "Select the center of the spinal cord" without the number at the beginning.
  2. When we reach the bottom of the image, the latest point is not displayed and there is no indication that the end has been reached.
  3. The first and last slices are not available in the automatic situation.

peristeri and others added 2 commits October 13, 2017 14:12
- Made sure that the GUI saved the points to file
- Set the initial auto point to 0
- When reaching the last valid point in auto mode, stay at that slice.
@jcohenadad
Copy link
Member

tested 5bcbd70:

  • init-mask: previous behaviour was that the first slice would be located in the middle slice (currently top slice)

@jcohenadad
Copy link
Member

@peristeri: your latest commit 1e37757 does not fix the issue i raised here: #1487 (comment).
i'll take care of it.

Copy link
Member

@jcohenadad jcohenadad left a comment

Choose a reason for hiding this comment

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

we can merge, but minor fixes will need to be addressed:
#1494

@jcohenadad jcohenadad dismissed benjamindeleener’s stale review October 14, 2017 11:41

i moved your requests in issue #1494 because we need to merge this PR ASAP

@jcohenadad jcohenadad merged commit deb96b9 into master Oct 14, 2017
@jcohenadad jcohenadad deleted the qt-propseg branch October 14, 2017 15:24
jcohenadad added a commit that referenced this pull request Oct 14, 2017
…x into jca_issue1452

* 'jca_issue1452' of github.com:neuropoly/spinalcordtoolbox:
  Used the absolute path to create the temporary label file in propseg (#1487)
jcohenadad added a commit that referenced this pull request Oct 27, 2017
* master: (43 commits)
  BUG: put back default value for labels (made batch_processing crash)
  BUG: replaced run(mkdir) with python’s makedirs
  raise in sct.run in bad order. Also added specific sct errors (#1503)
  Major refactoring of testing framework (#1453)
  More improvements to the viewer (#1496)
  DEV: add a line in the output excel that compute the percentage of a given tract occupied by lesionS: vol_lesionS_in_PAM50_04 / vol_PAM50_04_tot
  BUG: fix errors in create_atlas.m (#1499)
  Used the absolute path to create the temporary label file in propseg (#1487)
  Option to install SCT in development mode (#1491)
  Refactored WM atlas creation pipeline and improved documentation  (#1492)
  Updated bibliographic URL on ReadMe (#1489)
  Fix a regression bug.
  Added key bindings to the undo, save and help actions. (#1480)
  OPT: propseg: fixed issue 1484 (#1485)
  Fixed global dependency in sct_process_segmentation call (#1481)
  Introduced the zoom functionality to the anatomical canvas (#1477)
  Poq issue 1409 log (#1411)
  The minimal QT dialog  (#1379)
  Update README.md
  download_data: forgot to add pmj_models in the -d flag
  ...
jcohenadad added a commit that referenced this pull request Oct 27, 2017
* jca-release: (44 commits)
  updated CHANGES for 3.1
  BUG: put back default value for labels (made batch_processing crash)
  BUG: replaced run(mkdir) with python’s makedirs
  raise in sct.run in bad order. Also added specific sct errors (#1503)
  Major refactoring of testing framework (#1453)
  More improvements to the viewer (#1496)
  DEV: add a line in the output excel that compute the percentage of a given tract occupied by lesionS: vol_lesionS_in_PAM50_04 / vol_PAM50_04_tot
  BUG: fix errors in create_atlas.m (#1499)
  Used the absolute path to create the temporary label file in propseg (#1487)
  Option to install SCT in development mode (#1491)
  Refactored WM atlas creation pipeline and improved documentation  (#1492)
  Updated bibliographic URL on ReadMe (#1489)
  Fix a regression bug.
  Added key bindings to the undo, save and help actions. (#1480)
  OPT: propseg: fixed issue 1484 (#1485)
  Fixed global dependency in sct_process_segmentation call (#1481)
  Introduced the zoom functionality to the anatomical canvas (#1477)
  Poq issue 1409 log (#1411)
  The minimal QT dialog  (#1379)
  Update README.md
  ...

# Conflicts:
#	dev/straightening/sct_straighten_spinalcord_LargeFOVOutput.py
jcohenadad added a commit that referenced this pull request Dec 18, 2019
* jca-release: (44 commits)
  updated CHANGES for 3.1
  BUG: put back default value for labels (made batch_processing crash)
  BUG: replaced run(mkdir) with python’s makedirs
  raise in sct.run in bad order. Also added specific sct errors (#1503)
  Major refactoring of testing framework (#1453)
  More improvements to the viewer (#1496)
  DEV: add a line in the output excel that compute the percentage of a given tract occupied by lesionS: vol_lesionS_in_PAM50_04 / vol_PAM50_04_tot
  BUG: fix errors in create_atlas.m (#1499)
  Used the absolute path to create the temporary label file in propseg (#1487)
  Option to install SCT in development mode (#1491)
  Refactored WM atlas creation pipeline and improved documentation  (#1492)
  Updated bibliographic URL on ReadMe (#1489)
  Fix a regression bug.
  Added key bindings to the undo, save and help actions. (#1480)
  OPT: propseg: fixed issue 1484 (#1485)
  Fixed global dependency in sct_process_segmentation call (#1481)
  Introduced the zoom functionality to the anatomical canvas (#1477)
  Poq issue 1409 log (#1411)
  The minimal QT dialog  (#1379)
  Update README.md
  ...

# Conflicts:
#	dev/straightening/sct_straighten_spinalcord_LargeFOVOutput.py


Former-commit-id: 45518c6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug category: fixes an error in the code priority:HIGH
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants