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

Reconsider over-eager warning #76

Open
effigies opened this issue Jul 5, 2018 · 2 comments
Open

Reconsider over-eager warning #76

effigies opened this issue Jul 5, 2018 · 2 comments

Comments

@effigies
Copy link
Collaborator

effigies commented Jul 5, 2018

I suspect this is due to a refactoring in which directories, and not domains, are the objects iterated over, but this warning is too-easily triggered:

grabbit/grabbit/core.py

Lines 446 to 450 in a4eb518

if name in self.domains:
msg = ("Domain with name '{}' already exists; returning existing "
"Domain configuration.".format(name))
warnings.warn(msg)
return self.domains[name]

For example:

layout = gb.BIDSLayout([(bids_dir, 'bids'), (preproc_dir, ['bids', 'derivatives'])])

Because the 'bids' domain appears twice, this is warning is displayed. However, I expect this to be very common among multi-root use cases, so we should reconsider this warning.

I see three possibilities:

  1. Re-think the potentially conflicting behavior, given the new invocation patterns, and trigger in that case.
  2. Do some work to predict whether an actual conflict may arise, and trigger only if that is the case.
  3. Remove the warning because there are not actual conflicts.
@tyarkoni
Copy link
Member

tyarkoni commented Jul 5, 2018

Agreed on both the diagnosis and cause. I think at least in the short-term, we can probably go with your third option. I was probably overly cautious, and I don't think one will run into this situation in the real world very often (plus, when one does run into it, in most cases the two domains will be identical anyway, so nothing bad should happen). I'd be fine commenting it out and maybe leaving a comment as a TODO somewhere.

@effigies
Copy link
Collaborator Author

effigies commented Jul 5, 2018

I can do this next time I'm digging around in the code, but it's not a big enough deal for right now. Could be a good chance for a new contributor to get their feet wet, though.

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

No branches or pull requests

2 participants