Skip to content

incorrect detailed graphs being generated #2668

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

Closed
satra opened this issue Aug 1, 2018 · 11 comments · Fixed by #2669
Closed

incorrect detailed graphs being generated #2668

satra opened this issue Aug 1, 2018 · 11 comments · Fixed by #2669
Milestone

Comments

@satra
Copy link
Member

satra commented Aug 1, 2018

Summary

the detailed graph is not listing names of nodes appropriately resulting in incorrect graphs.

scroll down to see the multiple arrows containing nodes in the figures of the following notebook.
https://miykael.github.io/nipype_tutorial/notebooks/introduction_quickstart.html

an example here:

image

i suspect this has something to do with how __repr__ and __str__ are being used.

@oesteban, @effigies - any quick ideas before i look into the code?

Actual behavior

Expected behavior

should have printed the old style with multiple nodes.

see cell 14 in this notebook:
https://github.com/ReproNim/reproducible-imaging/blob/master/notebooks/introductory_dataflows.ipynb

How to replicate the behavior

run the current quickstart notebook in @miykael binder repo.

@djarecka
Copy link
Collaborator

djarecka commented Aug 1, 2018

I've just done some renaming, so it's now called introduction_quickstart_non-neuroimaging. I also built the new container for binder, can go here

@djarecka
Copy link
Collaborator

djarecka commented Aug 2, 2018

@oesteban - in tis PR you removed __repr__ and created __str__ in engine.base.py, but self.fullname doesn't have information about the iterables, so the difference between self.fullname and '.'.join((self._hierarchy, self._id)) can be:
'hello_mapnode.add_1' vs 'hello_mapnode.add_1.a1'

was this on purpose?

@oesteban
Copy link
Contributor

oesteban commented Aug 2, 2018

I can remember having issues with those three properties. If I recall correctly, iterable nodes were not returning the hierarchy with the fullname property. I tried to sort that out (with little success, seemingly).

@oesteban
Copy link
Contributor

oesteban commented Aug 2, 2018

And also, Workflows should not have itername, is that correct?

@djarecka
Copy link
Collaborator

djarecka commented Aug 2, 2018

@oesteban - I wasn't completely sure whether Workflow should have itername, it looks like it used to have. I moved back itername to EngineBase in #2669, but I can revert my changes if you believe that it's better to keep it in Node. Do you have a better solution to test_deep_nested_write_graph_runs fail?
We decided to use n.itername here, and that was working fine for Satra's tutorial and simpler examples, but not for nested workflows.

@oesteban
Copy link
Contributor

oesteban commented Aug 2, 2018

With 2.0 on the horizon I don't think it is worth your time finding out how to make this work the most rigorous way. If it works, I'm fine with it.

@djarecka
Copy link
Collaborator

djarecka commented Aug 2, 2018

@oesteban - ok, if anyone has any problems with naming, printing etc., we will review it

@effigies effigies added this to the 1.1.2 milestone Aug 3, 2018
@satra
Copy link
Member Author

satra commented Nov 1, 2018

seems like this is still the case:

scroll down a few cells from here: https://miykael.github.io/nipype_tutorial/notebooks/basic_joinnodes.html#Exercise-1

@miykael @djarecka - any thoughts.

@miykael
Copy link
Collaborator

miykael commented Nov 1, 2018

@satra, this is corrected in the docker image, but not in the figure on the webpage. The current setup doesn't create the webpage dynamically, i.e. with every new PR. It's me manually updating the page and especially the figures from time to time.

I chose this option because I didn't know better and because I also didn't want to have multiple image-diffs in the git log and make it explode with this. Do you think this is a reasonable concern or is there an alternative? I thought about generating the page via circleci and directly pushing it to a particular branch?

@satra
Copy link
Member Author

satra commented Nov 1, 2018

@miykael - i think doing something with circle would be good. just like dorota runs the tests on the repo everytime we release, we should update everything on the page, when we release. i think it would not explode with every release. and in some sense no worse off than the main notebook repo itself.

@effigies
Copy link
Member

effigies commented Nov 1, 2018

You can have a look at the nipybot stuff (tools/update_feedstock.sh + .circleci/config.yml) for submitting/updating PRs on release branches.

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 a pull request may close this issue.

5 participants