Skip to content
This repository has been archived by the owner on Oct 24, 2024. It is now read-only.

Backslash doesn't split to child trees #250

Closed
Illviljan opened this issue Jul 31, 2023 · 9 comments
Closed

Backslash doesn't split to child trees #250

Illviljan opened this issue Jul 31, 2023 · 9 comments
Labels
bug Something isn't working

Comments

@Illviljan
Copy link

# Forward slash works:
vertebrates = DataTree.from_dict(
    name="Vertebrae",
    d={
        "/Sharks": None,
        "/Bony Skeleton/Ray-finned Fish": None,
        "/Bony Skeleton/Four Limbs": None,
    },
)
vertebrates
Out[67]: 
DataTree('Vertebrae', parent=None)
├── DataTree('Sharks')
└── DataTree('Bony Skeleton')
    ├── DataTree('Ray-finned Fish')
    └── DataTree('Four Limbs')

# Backslash doesn't work:
vertebrates = DataTree.from_dict(
    name="Vertebrae",
    d={
        "\Sharks": None,
        "\Bony Skeleton\Ray-finned Fish": None,
        "\Bony Skeleton\Four Limbs": None,
    },
)
vertebrates
Out[69]: 
DataTree('Vertebrae', parent=None)
├── DataTree('\Sharks')
├── DataTree('\Bony Skeleton\Ray-finned Fish')
└── DataTree('\Bony Skeleton\Four Limbs')

I'm guessing this is a Windows vs. Linux thing? For example, there's PurePosixPath usage here:

class NodePath(PurePosixPath):

https://docs.python.org/3/library/pathlib.html#pathlib.PurePosixPath
https://docs.python.org/3/library/pathlib.html#pathlib.PureWindowsPath

I'm guessing adding a Windows CI might find some more issues.

@TomNicholas TomNicholas added the bug Something isn't working label Jul 31, 2023
@TomNicholas
Copy link
Member

Good catch. Yes I expect this is because I used PurePosixPath.

Q: Do you think backward slashes should be coerced into forward ones? Or an error raised? Presumably we do not want two valid separator characters?

@Illviljan
Copy link
Author

Illviljan commented Jul 31, 2023

Windows uses backslash by default and coerces forward slashes to backslashes, I think we should handle both.

I'm hoping pathlib has a good trick for this but I haven't looked in to this enough.
If not, maybe the separator can be os-dependent?

Edit:
But url:s uses forwardslashes (github.com//issues/250) so we should probably just be able to split on both.

@TomNicholas
Copy link
Member

If we're lucky then changing PurePosixPath to PurePath will mostly "just work".

@TomNicholas
Copy link
Member

But url:s uses forwardslashes (github.com/#250) so we should probably just be able to split on both.

Why do you say this? You're talking about a case where a datatree is created from a url, but on a windows machine?

@Illviljan
Copy link
Author

Illviljan commented Jul 31, 2023

Yes, I just noticed the urls on the browser is forward slashes on my windows computer. And I was thinking someone could experience that case as well.

@TomNicholas
Copy link
Member

Interesting. Is it possible to create a datatree from a url right now though? Where slashes in the url become levels in the tree?

@Illviljan
Copy link
Author

I don't know, I haven't tried it yet. The url-checks in xr.open_mfdataset came to mind though: https://github.com/pydata/xarray/blob/afab41d30b2692c350b51a1f69dd94129bb846cb/xarray/backends/common.py#L60-L65

@TomNicholas
Copy link
Member

TomNicholas commented Oct 23, 2023

Thinking about this again, I don't think we should support multiple separators in the node paths. The node paths have nothing to do with the filesystem, they don't represent files on disk.

We might however want to help the user when opening from or saving to multiple files, in which case on Windows we should convert backslashes in filepaths into forward slashes in node paths, and back again when saving. Until we actually have an open_mfdatatree or save_mfdatatree function we don't need to worry about this though.

@keewis
Copy link
Contributor

keewis commented Aug 13, 2024

I don't think we should support backslashes in datatree paths either, group paths should have nothing to do with filesystem paths (just like urls this should always be the same across operating systems).

Closing, but if you want to continue discussing please open a new issue on the xarray repository.

@keewis keewis closed this as completed Aug 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants