Skip to content

Conversation

@sciunto
Copy link
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
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
Member Author

sciunto commented Jun 6, 2016

Fixed and I corrected few other places.

@sciunto
Copy link
Member Author

sciunto commented Jun 6, 2016

and fixed again.

@soupault
Copy link
Member

soupault commented Jun 7, 2016

Here come the conflicts 😈.

@sciunto
Copy link
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
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
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
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
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
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
Member

ahojnnes commented Jun 8, 2016

@sciunto Travis fails.

@sciunto
Copy link
Member Author

sciunto commented Jun 9, 2016

@ahojnnes ? Travis is green for me.

@ahojnnes
Copy link
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

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
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
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
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
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
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
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
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
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