Skip to content

fix #85 : display dtype (in title) #86

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

Merged
merged 3 commits into from
Oct 11, 2017

Conversation

alixdamman
Copy link
Contributor

  • display shape + dtype of Numpy ndarray in title

No need to change Tooltip associated with listwidget. It uses array.info which will be changed in larray project (see PR larray-project/larray#459).

* display shape + dtype of Numpy ndarray in title
@alixdamman alixdamman requested a review from gdementen October 4, 2017 09:57
shape = ' x '.join('({})'.format(length) for length in array.shape)
dtype = ' [{}]'.format(array.dtype.name)
title += [(name + ': ' + shape + dtype) if name else shape + dtype]
# name of non-LArray/Numpy displayed item (if not None)
elif name:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this case is even possible now that you explicitly handle np.ndarray.

shape = ' x '.join('({})'.format(length) for length in array.shape)
dtype = ' [{}]'.format(array.dtype.name)
title += [(name + ': ' + shape + dtype) if name else shape + dtype]
# name of non-LArray/Numpy displayed item (if not None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not move the dtype thing outside this if statement:

if isinstance(array, LArray):
    ...
elif isinstance(array, np.ndarray):
   ...
else:
   ...

if isinstance(array, (LArray, np.ndarray)):
    title[-1] += ' [{}]'.format(array.dtype.name)

also anything we display in the viewer should have a shape (even if no .shape attribute), so you could refactor this into something like:

if isinstance(array, LArray):
    shape_parts = ["%s (%d)" % (display_name, len(axis)) for display_name, axis in zip(axes.display_names, axes)]
elif isinstance(array, np.ndarray):
    shape_parts = [str(l) for l in array.shape]
else:
    # unsure this is necessary, but we could add this case
    shape_parts = [str(len(array))]

array_info = ' x '.join(shape_parts)
if name:
   array_info = name + ': ' array_info
if isinstance(...):
    array_info += ' [{}]'.format(array.dtype.name)
full_title = ' - '.join([title_part for title_part in [fname, array_info, title] if title_part != ''])

BTW: I think that the unsaved_modification indicator would look better/be more standard if it was next to (just before) the filename, instead of at the end.

title = [name]
shape = ['{} ({})'.format(display_name, len(axis))
for display_name, axis in zip(array.axes.display_names, array.axes)]
# if it's not an LArray, it must be a Numpy ndarray
Copy link
Collaborator

Choose a reason for hiding this comment

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

adding an assert checking that assumption would probably be worth it in the long run, when we start displaying more stuff

@alixdamman alixdamman merged commit 180ad81 into larray-project:master Oct 11, 2017
@alixdamman alixdamman deleted the Display_dtype_85 branch October 11, 2017 07:20
gdementen pushed a commit to gdementen/larray-editor that referenced this pull request Oct 11, 2017
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.

2 participants