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

Relative data file path printing #79

Merged
merged 8 commits into from
Jan 26, 2019

Conversation

tashrifbillah
Copy link
Contributor

Fixes #78

@addisonElliott
Copy link
Collaborator

We might want to add some tests to fully verify that the new parameter is working as expected.

@codecov-io
Copy link

codecov-io commented Jan 25, 2019

Codecov Report

Merging #79 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #79   +/-   ##
=======================================
  Coverage   88.12%   88.12%           
=======================================
  Files           6        6           
  Lines         362      362           
  Branches      117      117           
=======================================
  Hits          319      319           
  Misses         21       21           
  Partials       22       22
Impacted Files Coverage Δ
nrrd/writer.py 87.17% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1c42d9...d820b43. Read the comment docs.

@tashrifbillah
Copy link
Contributor Author

tashrifbillah commented Jan 25, 2019

Added tests, two for absolute path, two for relative path. Basically, I modified the existing detached header writing tests in a clever way that encompasses new tests.

cc: @ihnorton

Copy link
Collaborator

@addisonElliott addisonElliott left a comment

Choose a reason for hiding this comment

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

Few minor issues, but overall I agree with what is being done.

@@ -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.

@@ -228,7 +228,7 @@ def test_write_detached_ascii(self):
data, header = nrrd.read(output_filename)
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.

nrrd/writer.py Outdated
@@ -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.

nrrd/writer.py Outdated
@@ -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.

Copy link
Collaborator

@addisonElliott addisonElliott left a comment

Choose a reason for hiding this comment

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

Few more minor changes, sorry!

In addition, do you mind just quickly reviewing my other PRs. Look for any issues you may see with the code. I should've split it up into multiple PRs but it was just a spring cleaning type of thing.

@@ -228,7 +230,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.

nrrd/writer.py Outdated
@@ -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 a relative path or absolute path.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, rereading this now I agree with how it's set.

Just one minor thing, I usually prefer the use of filename rather than file name. To stay consistent let's change it. I think I have it without the space in the docs and such.

Suggested change
Whether the data file name in detached header is saved with a relative path or absolute path.
Whether the data filename in detached header is saved with a relative path or absolute path.

@addisonElliott addisonElliott merged commit 017a905 into mhe:master Jan 26, 2019
@addisonElliott
Copy link
Collaborator

@tashrifbillah No problem, we're all busy so I understand if it takes you some time.

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.

3 participants