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

Domain root should default to directory containing config file #57

Closed
kleinschmidt opened this issue Apr 6, 2018 · 6 comments · Fixed by #63
Closed

Domain root should default to directory containing config file #57

kleinschmidt opened this issue Apr 6, 2018 · 6 comments · Fixed by #63
Labels

Comments

@kleinschmidt
Copy link
Contributor

It looks like if no root is specified in a directory-specific domain (or if root == '.') then the root of that domain is set to the global (Layout-level) root. I'm confused by this behavior. I'd have intuitively thought that the domain's root would be the directory it's config file is taken from by default, and that relative paths would be relative to that directory, rather than the layout root. In fact, it seems to go against the purpose of the domains mechanisms to have to specify the relative path of the domain from the layout root in the domain-specific config file, when that information should be taken from the location of that file (by default). Am I mis-understanding something?

As an example, adapted from the examples notebook:

stamps = gb.Layout('../grabbit/tests/data/valuable_stamps/', absolute_paths=False, config_filename='dir_config.json', config='../grabbit/tests/specs/stamps.json')
[(k, d.root) for (k, d) in stamps.domains.items()]

gives

[('stamps', '../grabbit/tests/data/valuable_stamps/'),
 ('usa_stamps', '../grabbit/tests/data/valuable_stamps/')]

while I would have expected

[('stamps', '../grabbit/tests/data/valuable_stamps/'),
 ('usa_stamps', '../grabbit/tests/data/valuable_stamps/USA/')]
@tyarkoni
Copy link
Member

tyarkoni commented Apr 6, 2018

You're right, I'd file this under bug. PR is welcome, otherwise I'll get to it when I can. Thanks for catching that!

@tyarkoni tyarkoni added the bug label Apr 6, 2018
@kleinschmidt
Copy link
Contributor Author

I'll file a PR then!

@tyarkoni tyarkoni changed the title confused about default Domain root Domain root should default to directory containing config file Apr 6, 2018
@tyarkoni
Copy link
Member

tyarkoni commented Apr 6, 2018

Actually I'm having to fix this in the process of fixing bids-standard/pybids#164, so you can probably hold off. My apologies if you've already started.

@tyarkoni
Copy link
Member

tyarkoni commented Apr 6, 2018

Actually, scratch that--your fix is more sensible than mine. Assuming tests pass, I'll merge yours and then work from that.

tyarkoni added a commit that referenced this issue Apr 6, 2018
WIP: correct relative domain roots #57
@tyarkoni tyarkoni reopened this Apr 19, 2018
@tyarkoni
Copy link
Member

I'm revisiting this due to some other issues that have come up, in the hopes of coming up with a better and more general solution. The way I see it, what's causing problems is the reliance on a project-wide root. If we get rid of this entirely, and mandate that each Domain must have its own root, I think that will simplify things. From an API standpoint, the main change (and I think it may be a breaking one for some use cases) is that it will now be mandatory for every Domain to have a specified root. This means that one of the following must be true: (i) there is a .root entry in the config file; (ii) the config file is to be read from a provided json file, in which case the directory the config is in will be used as the root; or (iii) the root argument must be passed when a Domain is initialized.

As a separate but related issue, I would also like to allow root to be a list, as we're running into situations where we need to apply the same set of rules to non-nested folders. I haven't explored the implications of this yet, and it may create a bit of ambiguity in a few places in the codebase (e.g., having to take the first entry in the list in cases where only one root can be used), but it seems necessary in order to have a framework that produces the kind of behavior a user would expect.

@kleinschmidt, any thoughts on the above? Unless you have objections, I'll try to work this up today.

@kleinschmidt
Copy link
Contributor Author

I think yoking roots to domains seems pretty natural. It's what I was planning to do in my julia clone, although I haven't implemented that writeable functionality so haven't needed to look at that. I also considered making the directory structure a more strict constraint on how to domains/queries nest, whereby domains are strictly contained within a parent domain, in which case the root would have to be relative (except for the top-level domain), and entities/tags would match against directory or filenames, not whole paths; those choices seem somehow related to me but I haven't given it a ton of thought.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants