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

MAINT: Refactoring the way nipype checks Node hashes #2383

Merged
merged 8 commits into from
Jan 23, 2018

Conversation

oesteban
Copy link
Contributor

Hopefully, to make it a bit simpler. This could affect #2380.

Main ideas:

  • Add a new Node.is_cached() member, trying to remove the responsibility of checking hashes from teh Plugins and putting it all in the Node context.
  • Simplify Node.hash_value() with the use of is_cached().
  • Node.needed_outputs is now a property (allowing some checks when setting).
  • Coordinate get_inputs, hash calculation, needed_outputs calculation and the hash elements: _hashvalue and _hashed_inputs

@oesteban oesteban changed the title MAINT: Refactoring the way nipype checks Node hashes WIP,MAINT: Refactoring the way nipype checks Node hashes Jan 19, 2018
@oesteban oesteban changed the title WIP,MAINT: Refactoring the way nipype checks Node hashes MAINT: Refactoring the way nipype checks Node hashes Jan 19, 2018
@oesteban oesteban requested a review from satra January 19, 2018 21:54
@oesteban
Copy link
Contributor Author

Maybe not necessary for this release, if you guys are very busy (as I imagine), but this should simplify hashing nodes. It'd be great to hear from @satra on this.

@effigies
Copy link
Member

I don't feel capable of reviewing this before 1.0. @mgxd @satra if you do, please tag it with the 1.0 milestone.

oesteban added a commit to nipreps/niworkflows that referenced this pull request Jan 19, 2018
oesteban added a commit to nipreps/niworkflows that referenced this pull request Jan 19, 2018
@satra
Copy link
Member

satra commented Jan 23, 2018

@oesteban - this looks good to me.

@satra
Copy link
Member

satra commented Jan 23, 2018

@effigies - your call if you want to include this in 1.0. it will make the code look a lot cleaner.

@satra
Copy link
Member

satra commented Jan 23, 2018

@effigies - it would be fine with me to include this.

@effigies
Copy link
Member

No objections from me, I just haven't reviewed it, myself. Feel free to merge.

@satra satra merged commit a2d054a into nipy:master Jan 23, 2018
@oesteban oesteban deleted the maint/node-hash branch January 23, 2018 03:38
@effigies effigies added this to the 1.0 milestone Aug 30, 2018
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