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.

Sign up for GitHub

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

Jump to bottom

Relative data file path printing #79

Merged
merged 8 commits into from
Jan 26, 2019
Merged

Relative data file path printing #79

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d590ed4
Merge pull request #1 from mhe/master
tashrifbillah Jan 24, 2019
bfc7db9
relative data path
tashrifbillah Jan 24, 2019
362612b
relative data path
tashrifbillah Jan 24, 2019
106d2f7
relative path test
tashrifbillah Jan 25, 2019
ae816aa
using relative filename
tashrifbillah Jan 26, 2019
524100f
wrapping lines at 120 characters
tashrifbillah Jan 26, 2019
fcb53fa
filename instead of file name
tashrifbillah Jan 26, 2019
d820b43
unused variable removal
tashrifbillah Jan 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions nrrd/tests/test_writing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ def test_write_detached_raw_as_nrrd(self):
output_filename = os.path.join(self.temp_write_dir, 'testfile_detached_raw.nhdr')
output_data_filename = os.path.join(self.temp_write_dir, 'testfile_detached_raw.nrrd')

nrrd.write(output_data_filename, self.data_input, {u'encoding': 'raw'}, detached_header=True)
nrrd.write(output_data_filename, self.data_input, {u'encoding': 'raw'}, detached_header=True, relative_data_path= False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
nrrd.write(output_data_filename, self.data_input, {u'encoding': 'raw'}, detached_header=True, relative_data_path= False)
nrrd.write(output_data_filename, self.data_input, {u'encoding': 'raw'}, detached_header=True, relative_data_path=False)

This line is > 120 characters, can you break it at this argument. Repeat for the one other case. Remove the additional space like shown.

Sorry, never explicitly stated that the syntax is to keep lines less than 120 characters, but it's just the standard I use.


# Read back the same file
data, header = nrrd.read(output_filename)
Expand Down Expand Up @@ -198,7 +198,7 @@ def test_write_detached_gz(self):
output_filename = os.path.join(self.temp_write_dir, 'testfile_detached_raw.nhdr')
output_data_filename = os.path.join(self.temp_write_dir, 'testfile_detached_raw.raw.gz')

nrrd.write(output_filename, self.data_input, {u'encoding': 'gz'}, detached_header=False)
nrrd.write(output_filename, self.data_input, {u'encoding': 'gz'}, detached_header=False, relative_data_path=False)

# Read back the same file
data, header = nrrd.read(output_filename)
Expand All @@ -216,7 +216,7 @@ def test_write_detached_bz2(self):
data, header = nrrd.read(output_filename)
self.assertEqual(self.expected_data, data.tostring(order='F'))
self.assertEqual(header['encoding'], 'bz2')
self.assertEqual(header['data file'], output_data_filename)
self.assertEqual(header['data file'], os.path.basename(output_data_filename))

def test_write_detached_ascii(self):
output_filename = os.path.join(self.temp_write_dir, 'testfile_detached_raw.nhdr')
Expand All @@ -228,7 +228,7 @@ def test_write_detached_ascii(self):
data, header = nrrd.read(output_filename)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, one small thing. Since you directly placed the string to be asserted, the output_data_filename variable is not used. Remove that please

Same for above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will review your other PRs, but may take some time.

self.assertEqual(self.expected_data, data.tostring(order='F'))
self.assertEqual(header['encoding'], 'txt')
self.assertEqual(header['data file'], output_data_filename)
self.assertEqual(header['data file'], os.path.basename(output_data_filename))
Copy link
Collaborator

Choose a reason for hiding this comment

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

output_data_filename is equal to joined path of temp dir and the filename. We then do the basename to get the filename.

Easier if we just replace output_data_filename with the relative filename.

Repeat for above case too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, didn't see the relative filename exists here.



if __name__ == '__main__':
Expand Down
9 changes: 6 additions & 3 deletions nrrd/writer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ def _format_field_value(value, field_type):
raise NRRDError('Invalid field type given: %s' % field_type)


def write(filename, data, header={}, detached_header=False, custom_field_map=None,
def write(filename, data, header={}, detached_header=False, relative_data_path=True, custom_field_map=None,
compression_level = 9):
"""Write :class:`numpy.ndarray` to NRRD file

Expand Down Expand Up @@ -121,6 +121,9 @@ def write(filename, data, header={}, detached_header=False, custom_field_map=Non
Data to save to the NRRD file
detached_header : :obj:`bool`, optional
Whether the header and data should be saved in separate files. Defaults to :obj:`False`
relative_data_path : :class:`bool`
whether the data file name in detached_header is saved with relative path
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line wrapped at 82 characters, only wrap at 120 characters.

Capitalize W in whether.

detached_header should be detached header because you are not referencing the boolean variable but rather the actual detached header.

I think we should include a note that this parameter is ignored if the data is attached just to be safe.

And, you say that the "data file name is saved with a relative path", but that sounds odd. The data filename isn't what is saved as a relative path, it's the data file itself.

Altogether, here's what I'm thinking. Comments welcome

Whether the data in a detached header is saved as a relative path or absolute path. This parameter is ignored if the data file is attached to the header file. Defaults to :obj:`True`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that is odd, rather the suggested one sounds like because data is not saved in the detached header. However, counter suggestion combining with yours:

Whether the data file name in detached header is saved with a relative path or absolute path.
This parameter is ignored if there is no detached header. Defaults to :obj:True

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR is ready, let me know your final thoughts on this.

or absolute path. Defaults to :obj:`True`
custom_field_map : :class:`dict` (:class:`str`, :class:`str`), optional
Dictionary used for parsing custom field types where the key is the custom field name and the value is a
string identifying datatype for the custom field.
Expand Down Expand Up @@ -183,12 +186,12 @@ def write(filename, data, header={}, detached_header=False, custom_field_map=Non
else:
raise NRRDError('Invalid encoding specification while writing NRRD file: %s' % header['encoding'])

header['data file'] = data_filename
header['data file'] = os.path.basename(data_filename) if relative_data_path else os.path.abspath(data_filename)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wrap at 120 characters please, below line is fine at just under 120.

else:
data_filename = header['data file']
elif filename.endswith('.nrrd') and detached_header:
data_filename = filename
header['data file'] = data_filename
header['data file'] = os.path.basename(data_filename) if relative_data_path else os.path.abspath(data_filename)
filename = '%s.nhdr' % os.path.splitext(filename)[0]
else:
# Write header & data as one file
Expand Down