Skip to content

ENH: Allow chained name_source #938

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

Merged
merged 9 commits into from
Feb 15, 2015
Merged

Conversation

oesteban
Copy link
Contributor

@oesteban oesteban commented Oct 7, 2014

An input trait with name_source defined and pointing to another trait that defines name_source pointing to a third trait now is sequentially resolved, adding the appropriate suffixes.

  • Improve former behavior
  • Add regression tests for name_sources:
    • With a 3 elements chain of name_sources
    • With a 3 elements ring of name_sources (raises NipypeInterfaceError)
    • With a 3 elements ring of name_sources, one is set and therefore the ring can be solved. Also checks that NipypInterfaceError is not raised.
  • Update documentation accordingly

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 2b1d259 on oesteban:enh/ChainedNameSource into 6da8f17 on nipy:master.

@satra
Copy link
Member

satra commented Oct 7, 2014

we should definitely add tests for this.

ns = trait_spec.name_source
while isinstance(ns, list):
if len(ns) > 1:
iflogger.warn('Only one name_source per trait is allowed')
Copy link
Member

Choose a reason for hiding this comment

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

why is only one allowed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, we have a somewhat difficult decision here.

Former implementation iterated the name_source list until it found the first defined name_source.
Current implementation may accept undefined name_sources, but they must have a name_source as well.

In both implementations only one name_source is actually used, right?.

@oesteban oesteban changed the title [engine] Allow chained name_source [WIP] Allow chained name_source Oct 13, 2014
@coveralls
Copy link

Coverage Status

Coverage increased (+0.35%) when pulling 06c2298 on oesteban:enh/ChainedNameSource into 6da8f17 on nipy:master.

@oesteban oesteban changed the title [WIP] Allow chained name_source WIP: Allow chained name_source Oct 16, 2014
@oesteban oesteban changed the title WIP: Allow chained name_source ENH: Allow chained name_source Feb 11, 2015
@oesteban
Copy link
Contributor Author

Hi @satra, I think this PR is ready to be considered. I've updated the description at the top, and the WIP flag replaced.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 70.6% when pulling b43941c on oesteban:enh/ChainedNameSource into ee31238 on nipy:master.

satra added a commit that referenced this pull request Feb 15, 2015
@satra satra merged commit 7a464e3 into nipy:master Feb 15, 2015
@oesteban oesteban deleted the enh/ChainedNameSource branch February 16, 2015 09:40
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