No longer concatenating extensions #2268

TheChymera · 2017-10-31T16:36:34Z

graph.png and graph.dot side by side is a lot better than graph.dot.png and graph.dot - for both tab completion, and software which has trouble intelligently parsing extensions (e.g. LaTeX's \includegraphics).

oesteban · 2017-10-31T18:26:57Z

You'll need to merge upstream/master for the tests to pass.

TheChymera · 2017-10-31T19:12:18Z

@oesteban what's upstream/master? Isn't nipy:master the main branch?

oesteban · 2017-10-31T20:35:54Z

sorry @TheChymera, it is pretty standard to call upstream to the original repo (nipy/nipype). If so, you normally do:

git fetch upstream
git merge upstream/master

But yes, that should be adapted to whatever your remote was called (or maybe you get the same result via a different solution, which is also valid of course).

EDIT - source: https://help.github.com/articles/syncing-a-fork/

TheChymera · 2017-10-31T22:19:45Z

@oesteban sorry, I though I already pulled the newest master before creating the branch (apparently I forgot), and I thought you were talking about some patch branch I am unaware of. Should be good now.

TheChymera · 2017-11-01T02:28:09Z

@oesteban very strange, it still fails, and it also indicates that the previous commit was also failing: https://github.com/TheChymera/nipype/commits/png

oesteban · 2017-11-01T03:00:56Z

Well, the previous commit failed on Travis. And actually, only one build of the matrix failed for a temporary error of the apt system.

This error seems related to the changes in this PR. If you check the log of the failing build, you'll see that failures pop up when buiding documentation with:

docker run --rm=false -it -v /home/ubuntu/work:/work -w /src/nipype/doc --entrypoint=/usr/bin/run_builddocs.sh nipype/nipype:py36 /usr/bin/run_builddocs.sh

The documentation writes graphs for the workflows, so I guess that is what is now broken.

codecov-io · 2017-11-01T11:54:26Z

Codecov Report

❗ No coverage uploaded for pull request base (master@80d3f05). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master    #2268   +/-   ##
=========================================
  Coverage          ?   52.15%           
=========================================
  Files             ?      122           
  Lines             ?    24380           
  Branches          ?        0           
=========================================
  Hits              ?    12715           
  Misses            ?    11665           
  Partials          ?        0

Flag	Coverage Δ
#smoketests	`52.15% <0%> (?)`

Impacted Files	Coverage Δ
nipype/pipeline/engine/utils.py	`57.92% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80d3f05...19bb33b. Read the comment docs.

TheChymera · 2017-11-01T15:02:11Z

@oesteban ok, I figured out and fixed the initial issue. Any idea what on earth is going on here? this certainly cannot decrease coverage by 20%.

Also, the travis build is failing due to what appears to be a network error:

The command "curl -s -o python-3.4.tar.bz2 ${archive_url}" failed and exited with 56 during .

Can you please restart it?

As for CircleCI - I cannot understand what's wrong with it...

oesteban · 2017-12-01T18:53:21Z

Traceback (most recent call last):

  File "../tools/build_interface_docs.py", line 57, in <module>

    docwriter.write_api_docs(outdir)

  File "/src/nipype/tools/interfacedocgen.py", line 496, in write_api_docs

    self.write_modules_api(modules, outdir)

  File "/src/nipype/tools/interfacedocgen.py", line 439, in write_modules_api

    api_str = self.generate_api_doc(m)

  File "/src/nipype/tools/interfacedocgen.py", line 334, in generate_api_doc

    ad += self._write_graph_section(fname, 'Graph') + '\n'

  File "/src/nipype/tools/interfacedocgen.py", line 231, in _write_graph_section

    os.remove(fname + ".png")

FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmpo7g48eb7.dot.png'

Makefile:37: recipe for target 'api' failed

make: *** [api] Error 1

mgxd · 2018-01-08T21:27:28Z

@TheChymera your PR changes "X.dot.png" to "X.dot", so you'll need to change

  File "/src/nipype/tools/interfacedocgen.py", line 231, in _write_graph_section

    os.remove(fname + ".png")

since it is still looking for the old concatenated extension '/tmp/tmpo7g48eb7.dot.png'

TheChymera · 2018-01-11T11:10:26Z

sorry for taking so long :-/

The build is failing during Conda install, again, I think this is unrelated to my PR:

Failed to fetch store:/var/lib/apt/lists/partial/repo.mongodb.org_apt_ubuntu_dists_trusty_mongodb-org_3.4_multiverse_binary-i386_Packages  Empty files can't be valid archives

TheChymera · 2018-01-13T08:38:48Z

@oesteban @mgxd can you maybe restart the build?

djarecka · 2018-01-13T12:53:38Z

@TheChymera - i've restarted travis, but couldn't restart circle since it looks like it 's run under your name. Are you sure you can't do it from you account?

TheChymera · 2018-01-13T15:52:33Z

@djarecka It seems travis is still failing due to some conda issues :(

Regarding circleCI, it says I need write permissions to trigger builds.

djarecka · 2018-01-13T22:45:43Z

@TheChymera - I've retried the 2 travis runs and I can see that someone started circleCI. Were you logged in when you were having problems with rebuilding? I really couldn't triggered it, and I can rebuild the ones run under nipype

TheChymera · 2018-01-14T23:24:13Z

I can trigger it now, yes, and I already triggered it once before

TheChymera · 2018-01-15T07:42:32Z

ok, so circleCI still fails, but I am certain this has nothing to do with my PR:

076     % hrf  - hemodynamic response function
077     % p    - parameters of the response function
078 
079     the following code using scipy.stats.distributions.gamma
080     doesn't return the same result as the spm_Gpdf function ::
081 
082         hrf = gamma.pdf(u, p[0]/p[2], scale=dt/p[2]) -
083               gamma.pdf(u, p[1]/p[3], scale=dt/p[3])/p[4]
084 
085     >>> print(spm_hrf(2))
Differences (unified diff with -expected +actual):
    @@ -1,5 +1,5 @@
    -[  0.00000000e+00   8.65660810e-02   3.74888236e-01   3.84923382e-01
    -   2.16117316e-01   7.68695653e-02   1.62017720e-03  -3.06078117e-02
    -  -3.73060781e-02  -3.08373716e-02  -2.05161334e-02  -1.16441637e-02
    -  -5.82063147e-03  -2.61854250e-03  -1.07732374e-03  -4.10443522e-04
    -  -1.46257507e-04]
    +[ 0.00000000e+00  8.65660810e-02  3.74888236e-01  3.84923382e-01
    +  2.16117316e-01  7.68695653e-02  1.62017720e-03 -3.06078117e-02
    + -3.73060781e-02 -3.08373716e-02 -2.05161334e-02 -1.16441637e-02
    + -5.82063147e-03 -2.61854250e-03 -1.07732374e-03 -4.10443522e-04
    + -1.46257507e-04]

/src/nipype/nipype/algorithms/modelgen.py:85: DocTestFailure

Can I merge?

oesteban

Couple of stylistic comments.

The failing test in Circle seems related to a missing NORMALIZE_WHITESPACE directive. Why would this fail here but not in master?

oesteban · 2018-01-16T01:27:54Z

nipype/pipeline/engine/utils.py

@@ -1055,7 +1055,8 @@ def export_graph(graph_in, base_dir=None, show=False, use_execgraph=False,
 def format_dot(dotfilename, format='png'):
    """Dump a directed graph (Linux only; install via `brew` on OSX)"""
    if format != 'dot':
-        cmd = 'dot -T%s -O \'%s\'' % (format, dotfilename)
+        formatted_dotfilename = os.path.splitext(dotfilename)[0]+'.'+format


~~spaces around + symbols~~ see comment below that would remove this line.

oesteban · 2018-01-16T01:28:14Z

tools/interfacedocgen.py

@@ -228,7 +228,8 @@ def _write_graph_section(self, fname, title):

        fhandle.close()
        os.remove(fname)
-        os.remove(fname + ".png")
+        bitmap_fname = os.path.splitext(dotfilename)[0]+'.png'


spaces around + symbol (even though formatting would be better):

bitmap_fname = '%s.png' % os.path.splitext(dotfilename)[0]

oesteban · 2018-01-16T01:30:47Z

nipype/pipeline/engine/utils.py

@@ -1055,7 +1055,8 @@ def export_graph(graph_in, base_dir=None, show=False, use_execgraph=False,
 def format_dot(dotfilename, format='png'):
    """Dump a directed graph (Linux only; install via `brew` on OSX)"""
    if format != 'dot':
-        cmd = 'dot -T%s -O \'%s\'' % (format, dotfilename)
+        formatted_dotfilename = os.path.splitext(dotfilename)[0]+'.'+format
+        cmd = 'dot -T{} -o"{}" "{}"'.format(format, formatted_dotfilename, dotfilename)


why not:

cmd = 'dot -T{0} -o"{1}.{0}" "{2}"'.format(format, os.path.splitext(dotfilename)[0], dotfilename)

and avoid the concatenation?

You need to add the indexes of the argument for format to use the first argument twice without doubling it explicitly. (ref. Chris' comment below)

TheChymera · 2018-01-16T12:11:41Z

The failing test in Circle seems related to a missing NORMALIZE_WHITESPACE directive. Why would this fail here but not in master?

@oesteban we could pull and find out?

TheChymera · 2018-01-18T01:24:27Z

Can I finally merge this? It's a fairly minor change, and it's already taken forever, with the build failing due to some obviously unrelated issue.

oesteban · 2018-01-18T02:13:04Z

That would break master. I don't think it would be justified.

I agree on that the failing test is probably not related to your changes. But it is telling us of something potentially wrong with nipype, and we should investigate that first.

oesteban · 2018-01-18T03:44:15Z

Why your tests still use Circle 1.0? We switched to 2.0 a while ago. Seems like your clone of the repo did not merge the new /.circle folder.

effigies · 2018-01-18T21:56:48Z

I've merged in master. If tests pass, are we good to merge?

effigies · 2018-01-19T00:31:17Z

nipype/pipeline/engine/utils.py

@@ -1316,7 +1316,8 @@ def export_graph(graph_in,
 def format_dot(dotfilename, format='png'):
    """Dump a directed graph (Linux only; install via `brew` on OSX)"""
    if format != 'dot':
-        cmd = 'dot -T%s -O \'%s\'' % (format, dotfilename)
+        formatted_dotfilename = +'.'+format


Syntax error here.

TheChymera · 2018-01-19T15:53:41Z

git blew up in my face, new try: #2381

TheChymera force-pushed the png branch from 3f33497 to 2f914bd Compare October 31, 2017 21:48

TheChymera force-pushed the png branch from 2f914bd to 6de40fa Compare November 1, 2017 11:54

mgxd added this to the 0.14.1 milestone Jan 8, 2018

TheChymera force-pushed the png branch from 6de40fa to 38f5ddd Compare January 11, 2018 10:02

oesteban reviewed Jan 16, 2018

View reviewed changes

No longer concatenating extensions

58a3009

TheChymera force-pushed the png branch from 38f5ddd to 58a3009 Compare January 16, 2018 02:15

Merge remote-tracking branch 'upstream/master' into png

19bb33b

effigies reviewed Jan 19, 2018

View reviewed changes

TheChymera mentioned this pull request Jan 19, 2018

No longer concatenating extensions #2381

Merged

TheChymera closed this Jan 19, 2018

TheChymera deleted the png branch January 19, 2018 16:01

effigies modified the milestones: 0.14.1, 1.0 Jan 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No longer concatenating extensions #2268

No longer concatenating extensions #2268

TheChymera commented Oct 31, 2017

oesteban commented Oct 31, 2017

TheChymera commented Oct 31, 2017

oesteban commented Oct 31, 2017 •

edited

Loading

TheChymera commented Oct 31, 2017

TheChymera commented Nov 1, 2017

oesteban commented Nov 1, 2017

codecov-io commented Nov 1, 2017 •

edited

Loading

TheChymera commented Nov 1, 2017

oesteban commented Dec 1, 2017

mgxd commented Jan 8, 2018

TheChymera commented Jan 11, 2018

TheChymera commented Jan 13, 2018

djarecka commented Jan 13, 2018

TheChymera commented Jan 13, 2018

djarecka commented Jan 13, 2018

TheChymera commented Jan 14, 2018

TheChymera commented Jan 15, 2018

oesteban left a comment

oesteban Jan 16, 2018

oesteban Jan 16, 2018

oesteban Jan 16, 2018

oesteban Jan 19, 2018

TheChymera commented Jan 16, 2018

TheChymera commented Jan 18, 2018

oesteban commented Jan 18, 2018

oesteban commented Jan 18, 2018

effigies commented Jan 18, 2018

effigies Jan 19, 2018

TheChymera commented Jan 19, 2018

No longer concatenating extensions #2268

No longer concatenating extensions #2268

Conversation

TheChymera commented Oct 31, 2017

oesteban commented Oct 31, 2017

TheChymera commented Oct 31, 2017

oesteban commented Oct 31, 2017 • edited Loading

TheChymera commented Oct 31, 2017

TheChymera commented Nov 1, 2017

oesteban commented Nov 1, 2017

codecov-io commented Nov 1, 2017 • edited Loading

Codecov Report

TheChymera commented Nov 1, 2017

oesteban commented Dec 1, 2017

mgxd commented Jan 8, 2018

TheChymera commented Jan 11, 2018

TheChymera commented Jan 13, 2018

djarecka commented Jan 13, 2018

TheChymera commented Jan 13, 2018

djarecka commented Jan 13, 2018

TheChymera commented Jan 14, 2018

TheChymera commented Jan 15, 2018

oesteban left a comment

Choose a reason for hiding this comment

oesteban Jan 16, 2018

Choose a reason for hiding this comment

oesteban Jan 16, 2018

Choose a reason for hiding this comment

oesteban Jan 16, 2018

Choose a reason for hiding this comment

oesteban Jan 19, 2018

Choose a reason for hiding this comment

TheChymera commented Jan 16, 2018

TheChymera commented Jan 18, 2018

oesteban commented Jan 18, 2018

oesteban commented Jan 18, 2018

effigies commented Jan 18, 2018

effigies Jan 19, 2018

Choose a reason for hiding this comment

TheChymera commented Jan 19, 2018

oesteban commented Oct 31, 2017 •

edited

Loading

codecov-io commented Nov 1, 2017 •

edited

Loading