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

Tab names for slice tabs as file names. #2973

Merged
merged 4 commits into from
Mar 5, 2024

Conversation

maharshi-gor
Copy link
Member

@maharshi-gor maharshi-gor commented Nov 10, 2023

Changes

This PR introduces a data field saying file name

The behavior to observe:

  • On the page a field Filename got added with the file name next to it.

Screenshot from 2024-01-30 15-40-11

  • I have updated all the test cases accordingly to take care of file while unpacking the images data.

@skoudoro
Copy link
Member

Hi @maharshi-gor,

What is the status of this PR ? Can you provide a description and some picture (before/after)? I do not understand the title and not sure what this PR is supposed to do/fix.

Thank you in advance for your feedback

@maharshi-gor
Copy link
Member Author

Hi @skoudoro ,

I am waiting to test this PR on a Windows machine as the naming strategy relies on splitting the path based on '/'. I do not know how Python is handling the path for Windows. I tested this on a mac and a linux machine works perfectly fine.

I would attach a few screen shots of expectations

@maharshi-gor maharshi-gor changed the title Feature/tab names Tab names for slice tabs as file names. Dec 5, 2023
@maharshi-gor maharshi-gor self-assigned this Dec 5, 2023
@@ -443,7 +442,8 @@ def build_show(self, scene):
img_count = 0
synchronize_slices = check_img_shapes(self.images)
for img in self.images:
data, affine = img
data, affine, fname = img
fname = fname.split("/")[-1]
Copy link
Member Author

Choose a reason for hiding this comment

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

splitting the file path and extracting the name

Copy link
Member

Choose a reason for hiding this comment

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

Are you sure it works on windows? I will be surprised because they usually use \\. I recommend you to use os.path.basename. You can also look at the Pathlib python module.

@@ -460,19 +460,17 @@ def build_show(self, scene):
world_coords=self.world_coords,
percentiles=[0, 100])
self.__tabs.append(SlicesTab(
slices_viz, slice_id=img_count + 1,
force_render=self._show_force_render))
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed id parameter which only was providing count to present in the slice tab.

Copy link

codecov bot commented Dec 5, 2023

Codecov Report

Attention: Patch coverage is 92.18750% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 82.43%. Comparing base (529a3fd) to head (32fe9a1).
Report is 5 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2973      +/-   ##
==========================================
+ Coverage   82.34%   82.43%   +0.08%     
==========================================
  Files         146      146              
  Lines       20664    20707      +43     
  Branches     3328     3334       +6     
==========================================
+ Hits        17015    17069      +54     
+ Misses       2817     2805      -12     
- Partials      832      833       +1     
Files Coverage Δ
dipy/testing/decorators.py 77.55% <100.00%> (+0.95%) ⬆️
dipy/viz/horizon/app.py 47.16% <100.00%> (+1.61%) ⬆️
dipy/viz/horizon/util.py 98.18% <100.00%> (+0.50%) ⬆️
dipy/workflows/viz.py 85.32% <100.00%> (ø)
dipy/viz/horizon/tab/base.py 75.25% <95.23%> (+4.23%) ⬆️
dipy/viz/horizon/tab/slice.py 51.42% <77.77%> (+2.18%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Thanks @maharshi-gor,

See below some comments to fix before I try on my latptop.

Can you also confirm that this work only at the startup. if we resize the window, it does not refresh the filename, right ?

@@ -443,7 +442,8 @@ def build_show(self, scene):
img_count = 0
synchronize_slices = check_img_shapes(self.images)
for img in self.images:
data, affine = img
data, affine, fname = img
fname = fname.split("/")[-1]
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure it works on windows? I will be surprised because they usually use \\. I recommend you to use os.path.basename. You can also look at the Pathlib python module.

string
File name with the extension in it.
"""
if extension[0] != '.':
Copy link
Member

Choose a reason for hiding this comment

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

if not extension.startswith('.') instead

)

def _handle_title_overflow(self, title_text, title_block):
tab_text = remove_extension(title_text, 'nii')
Copy link
Member

Choose a reason for hiding this comment

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

Not sure you need a function for that....

tab_text = title_text.split('.', 1)[0] should just be enough.....

in pathlib module, you also have suffixes()

dipy/viz/horizon/tab/base.py Show resolved Hide resolved
for act in actors:
scene.rm(act)
scene.add(act)
def get_size(actor):
Copy link
Member

Choose a reason for hiding this comment

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

Why did you add this function? where do you use it ?

@maharshi-gor
Copy link
Member Author

Thanks @maharshi-gor,

See below some comments to fix before I try on my latptop.

Can you also confirm that this work only at the startup. if we resize the window, it does not refresh the filename, right ?

The resize window will required more changes. The current PR only targets the startup.

@maharshi-gor
Copy link
Member Author

Hello @skoudoro , I have resolved all the mentioned comments and also added a few test cases for the util file.

Thank you, for the thorough review!

commit 2170d22
Author: Maharshi Gor <gor.maharshi@gmail.com>
Date:   Tue Feb 20 10:40:45 2024 -0500

    scene removed from tab manager

commit 337bc2f
Author: Maharshi Gor <gor.maharshi@gmail.com>
Date:   Wed Jan 31 10:53:01 2024 -0500

    test case fix

commit 4ee6416
Author: Maharshi Gor <gor.maharshi@gmail.com>
Date:   Tue Jan 30 15:39:07 2024 -0500

    file name working

commit 64aa0c2
Author: Maharshi Gor <gor.maharshi@gmail.com>
Date:   Tue Jan 30 11:46:02 2024 -0500

    ellipsis test added

commit 99821ad
Merge: 1167146 7e1d10a
Author: Maharshi Gor <gor.maharshi@gmail.com>
Date:   Tue Jan 30 11:40:56 2024 -0500

    Merge branch 'master' into feature/tab-names

commit 1167146
Merge: eb501db cf636e7
Author: Maharshi Gor <gor.maharshi@gmail.com>
Date:   Mon Jan 29 14:05:37 2024 -0500

    Merge branch 'feature/tab-names' of github.com:maharshi-gor/dipy into feature/tab-names

commit cf636e7
Author: maharshigor <gor.maharshi@gmail.com>
Date:   Tue Dec 12 15:16:29 2023 -0500

    changes for horizon code case

commit 225a207
Merge: a3b1b42 b825c14
Author: maharshigor <gor.maharshi@gmail.com>
Date:   Tue Dec 12 14:24:01 2023 -0500

    Merge remote-tracking branch 'upstream/master' into feature/tab-names

commit a3b1b42
Author: Maharshi Gor <gor.maharshi@gmail.com>
Date:   Thu Dec 7 13:42:22 2023 -0500

    comments fixed and util test cases added

commit 063bcb8
Author: Maharshi Gor <gor.maharshi@gmail.com>
Date:   Tue Dec 5 12:28:56 2023 -0500

    Fix all test cases

commit 9fe236d
Author: Maharshi Gor <gor.maharshi@gmail.com>
Date:   Thu Nov 30 16:39:53 2023 -0500

    test cases fixed

commit 68da7d2
Author: Maharshi Gor <gor.maharshi@gmail.com>
Date:   Thu Nov 16 14:15:38 2023 -0500

    windows test left

commit fd49628
Author: Maharshi Gor <gor.maharshi@gmail.com>
Date:   Fri Nov 10 14:20:05 2023 -0500

    changes for tab names

commit e8ffcbf
Author: Maharshi Gor <gor.maharshi@gmail.com>
Date:   Fri Nov 10 13:02:44 2023 -0500

    tab names fixed

commit eb501db
Merge: 2fca5f0 1419292
Author: Maharshi Gor <gor.maharshi@gmail.com>
Date:   Fri Dec 8 17:20:49 2023 -0500

    Merge branch 'master' into feature/tab-names

commit 2fca5f0
Author: Maharshi Gor <gor.maharshi@gmail.com>
Date:   Thu Dec 7 13:42:22 2023 -0500

    comments fixed and util test cases added

commit 208d892
Author: Maharshi Gor <gor.maharshi@gmail.com>
Date:   Tue Dec 5 12:28:56 2023 -0500

    Fix all test cases

commit 5361104
Author: Maharshi Gor <gor.maharshi@gmail.com>
Date:   Thu Nov 30 16:39:53 2023 -0500

    test cases fixed

commit 898c106
Author: Maharshi Gor <gor.maharshi@gmail.com>
Date:   Thu Nov 16 14:15:38 2023 -0500

    windows test left

commit 07218a6
Merge: 269a459 f92f595
Author: Maharshi <39947025+maharshi-gor@users.noreply.github.com>
Date:   Sat Nov 11 10:41:26 2023 -0500

    Merge branch 'master' into feature/tab-names

commit 269a459
Author: Maharshi Gor <gor.maharshi@gmail.com>
Date:   Wed Nov 8 14:17:37 2023 -0500

    bugfix for --force issue

commit c929fac
Author: Maharshi Gor <gor.maharshi@gmail.com>
Date:   Tue Nov 7 16:22:53 2023 -0500

    opacity and slider toggle fixed

commit 41afb35
Author: Maharshi Gor <gor.maharshi@gmail.com>
Date:   Fri Nov 10 14:20:05 2023 -0500

    changes for tab names

commit e831925
Author: Maharshi Gor <gor.maharshi@gmail.com>
Date:   Fri Nov 10 13:02:44 2023 -0500

    tab names fixed
Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Hi @maharshi-gor,

Can you merge master on this PR or rebase to fix the conflicts.

is it normal to have 3 dots (ellipsis) at the beginning of the filename ? it is weird, see below the weird filename:

image

EDIT: the name should be mni_icbm152_t1_tal_nlin_asym_09c.nii

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

ok, all good, thanks @maharshi-gor!

merging

@skoudoro skoudoro merged commit e0cd7db into dipy:master Mar 5, 2024
28 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants