Skip to content

DOC: refactor axes with lists#2129

Merged
ahojnnes merged 1 commit intoscikit-image:masterfrom
sciunto:axes
Jun 9, 2016
Merged

DOC: refactor axes with lists#2129
ahojnnes merged 1 commit intoscikit-image:masterfrom
sciunto:axes

Conversation

@sciunto
Copy link
Copy Markdown
Member

@sciunto sciunto commented Jun 5, 2016

To avoid boilerplate code and to make things easier to modify.

@soupault soupault added the 🔧 type: Maintenance Refactoring and maintenance of internals label Jun 5, 2016
ax[3].imshow(labels, cmap=plt.cm.spectral, interpolation='nearest', alpha=.7)
ax[3].set_title("Segmented")

for a in axes:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

*axes -> ax

@sciunto
Copy link
Copy Markdown
Member Author

sciunto commented Jun 6, 2016

Fixed and I corrected few other places.

@sciunto
Copy link
Copy Markdown
Member Author

sciunto commented Jun 6, 2016

and fixed again.

@soupault
Copy link
Copy Markdown
Member

soupault commented Jun 7, 2016

Here come the conflicts 😈.

@sciunto
Copy link
Copy Markdown
Member Author

sciunto commented Jun 7, 2016

@soupault rebased 🎱

ax.imshow(zdh)
ax.set_title("Stain separated image (rescaled)")
ax.axis('off')
axis = plt.subplot(1, 1, 1, sharex=ax[0], sharey=ax[0], adjustable='box-forced')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should refactor this in further PRs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't understand your comment?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, I see, this is a leftover comment. Ignore.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ahojnnes I meant that 1, 1, 1, sharex=ax[0], sharey=ax[0] is redundant here. fig, axes might be initialized in a single line (same as in other examples).

@soupault
Copy link
Copy Markdown
Member

soupault commented Jun 7, 2016

👍

@soupault soupault added 📄 type: Documentation Updates, fixes and additions to documentation and removed 🔧 type: Maintenance Refactoring and maintenance of internals labels Jun 7, 2016
@ahojnnes
Copy link
Copy Markdown
Member

ahojnnes commented Jun 8, 2016

@sciunto Travis fails.

@sciunto
Copy link
Copy Markdown
Member Author

sciunto commented Jun 9, 2016

@ahojnnes ? Travis is green for me.

@ahojnnes
Copy link
Copy Markdown
Member

ahojnnes commented Jun 9, 2016

Interestingly it does for me now as wel. In it goes. Thanks.

@ahojnnes ahojnnes merged commit 31e8842 into scikit-image:master Jun 9, 2016
@codecov-io
Copy link
Copy Markdown

Current coverage is 90.50%

Merging #2129 into master will not change coverage

@@             master      #2129   diff @@
==========================================
  Files           297        297          
  Lines         21230      21230          
  Methods           0          0          
  Messages          0          0          
  Branches       1949       1949          
==========================================
  Hits          19215      19215          
+ Misses         1665       1661     -4   
- Partials        350        354     +4   

Powered by Codecov. Last updated by 47d0aa9...6ad792c

@soupault
Copy link
Copy Markdown
Member

soupault commented Jun 9, 2016

Just to clarify, there was no magic - I restarted the build for OS X (the one which was failing).

ax0.imshow(l_resized, extent=(0, 128, 128, 0), interpolation='nearest',
ax[0].set_title("Original rescaled with\n spline interpolation (order=3)")
ax[0].imshow(l_resized, extent=(0, 128, 128, 0), interpolation='nearest',
cmap=cm.Greys_r)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This broke PEP8

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'll quickly fix it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See #2136

@jni
Copy link
Copy Markdown
Member

jni commented Jun 9, 2016

Can someone explain to me how this reduces boilerplate? The bulk of this PR is every axn being replaced with ax[n]. imho this reduces maintainability because you're still hardcoding a value, but now it's harder to grep/sed.

@ahojnnes
Copy link
Copy Markdown
Member

ahojnnes commented Jun 9, 2016

Nope, but I don't care either way and thought this convention was agreed upon. I would argue that the majority of our examples use the ax[N] syntax, so rather than reducing boilerplate this increases consistency across the code base.

@jni
Copy link
Copy Markdown
Member

jni commented Jun 9, 2016

@ahojnnes if that's the case I'm ok. I assumed that this PR changed all examples from one convention to the other. Consistency is good! =)

@ahojnnes
Copy link
Copy Markdown
Member

ahojnnes commented Jun 9, 2016

@jni It's only 8 changed files and I hope we have more examples ;-)

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

Labels

📄 type: Documentation Updates, fixes and additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants