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

Print relative data file name in the header #78

Closed
tashrifbillah opened this issue Jan 23, 2019 · 6 comments
Closed

Print relative data file name in the header #78

tashrifbillah opened this issue Jan 23, 2019 · 6 comments

Comments

@tashrifbillah
Copy link
Contributor

In the detached header, the data file name is printed with abspath. That means, if someone moves the data around, than data file location is invalid. Instead, how about we write the data file with basename?

https://github.com/mhe/pynrrd/blob/master/nrrd/writer.py#L186 and https://github.com/mhe/pynrrd/blob/master/nrrd/writer.py#L191

I think the specification also suggests that:

Breaking the dataset into a header and one or more data files raises a new concerns, namely that the header file can't know if the data file has been erased, renamed, or moved. NRRD provides no means to overcome these problems once they've been created. On the other hand, moving the header and data files together to a new place is a common operation, and is supported by the special semantics associated with the data filename:
If the filename (given either directly from "filename", or generated from "format", or listed in a "LIST") does not begins with "/", it is taken to be the filename of the data file, relative to the location of the detached header file. The NRRD reader is responsible for constructing the full data filename from the filename of the detached header and "filename". Obviously, if the detached header has been passed to the reader not as a file name, but as a FILE* (such as stdin), this filename construction is impossible.
Otherwise, if "filename" does start with "/", then filename gives the full path of the data file. It is passed directly to fopen() without any interpretation.
Note: as of NRRD0004, the signifier of a header-relative file changed from the presence (at the beginning of the filename) of "./", to the absence of "/". Since essentially all uses of detached headers assumed header-relative data files, the explicit "./" flag was deemed unnecessary. With this change, it becomes impossible for a header to refer to the data file relative to the current working directory of the reader (which may be different that the directory of the header), but that's probably a good thing.

Suggestion:

header['data file']= os.path.basename(data_filename)

Happy to submit a PR.

cc: @ihnorton

@ihnorton
Copy link
Contributor

Yes, this sounds correct and matches the behavior of other readers.

@addisonElliott
Copy link
Collaborator

From the excerpt you quoted, it sounds like pynrrd is with the standard.

Current logic for pynrrd:

  • If filename is absolute (i.e. starts with / [not really true in Windows but whatever]), then we give an absolute header path.
  • If filename is relative, then we give a relative header path.

Probably 99% of the time you want a relative detached header so I understand this can be annoying.

Another argument could be added to nrrd.write such as detachedAbsolute or something to indicate whether to use absolute or relative path. However, the question becomes what the default value should be.

I don't want to break compatibility but my preference would be to set relative paths to true since that is used the most imo.

@tashrifbillah
Copy link
Contributor Author

I think you are talking about the reader, not the writer:

Current logic for pynrrd:

https://github.com/mhe/pynrrd/blob/master/nrrd/reader.py#L345

I am not arguing that pynrrd is defying any standard, which does not seem to be specified in the excerpt either.

However, unu save always prints the relative data file name in the detached header.

Again, I don't see how we may break compatibility if we write relative header name instead of absolute header name (even when user specifies one).

@addisonElliott
Copy link
Collaborator

No, the reader logic should be correct. The write logic is what I'm referring to.

I think I misinterpreted the excerpt you gave. I thought it was referring to the filename given when writing but I think it means the filename read from the data header. Thus, I agree that the standard does not specify.

I think the best option would be to create an argument in nrrd.write specifying whether or not to use an absolute or relative path. I think defaulting it to use relative paths would be okay.

@tashrifbillah
Copy link
Contributor Author

So, if defaulting to relative path is not breaking the reading afterwards i.e.

The NRRD reader is responsible for constructing the full data filename from the filename of the detached header and "filename"

do we need to add additional argument? Aren't we better off setting the 'data file' to relative path?

@addisonElliott
Copy link
Collaborator

I see what you're saying. Either way, absolute or relative paths, reading the NRRD will be fine assuming the paths are correct.

It is up to the user whether they want to print out an absolute or relative path for the detached filename. Personally, I'm of the philosophy that if you have an option that you can foresee someone wanting to change it, then make it an option and leave it up to the user.

I don't know a specific use case why someone would want to use an absolute path.

addisonElliott pushed a commit that referenced this issue Jan 26, 2019
Add argument `relative_data_path` to `nrrd.write` for specifying whether to save the 'data file' field in the header relative or absolute.

Fixes #78
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

No branches or pull requests

3 participants