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

Fix regression in scipy 1.5.0 in dendogram when labels is a numpy array. #12421

Merged
merged 3 commits into from Jun 25, 2020

Conversation

lesteve
Copy link
Contributor

@lesteve lesteve commented Jun 24, 2020

Reference issue

Fix #12418.

What does this implement/fix?

This fixes a regression when labels is a numpy array.

I added a test but I am not too familiar with dendrogram so let me know if the labels I chose do not make sense.

@lesteve lesteve changed the title Fix regression in dendogram when labels is a numpy array. Fix regression in scipy 1.5.0 in dendogram when labels is a numpy array. Jun 24, 2020
@AtsushiSakai AtsushiSakai added scipy.cluster defect A clear bug or issue that prevents SciPy from being installed or used as expected labels Jun 24, 2020
Copy link
Member

@AtsushiSakai AtsushiSakai left a comment

Choose a reason for hiding this comment

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

LGTM. I'm sorry my previous PR did not test ndarray.

@tylerjereddy tylerjereddy added this to the 1.6.0 milestone Jun 25, 2020
@tylerjereddy tylerjereddy added the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Jun 25, 2020
@tylerjereddy
Copy link
Contributor

I added the backport label and the 1.6.0 milestone so I can easily find this PR for backporting to 1.5.1 when it is ready & merged.

@@ -812,6 +812,13 @@ def test_valid_orientation(self):
Z = linkage(hierarchy_test_data.ytdist, 'single')
assert_raises(ValueError, dendrogram, Z, orientation="foo")

def test_can_take_label_as_array_or_list(self):
Z = linkage(hierarchy_test_data.ytdist, 'single')
labels = np.array([1, 3, 2, 6, 4, 5])
Copy link
Contributor

Choose a reason for hiding this comment

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

testing with an empty NumPy array might also be useful, though maybe not required

adding a comment to the test like # test for gh-12418 can be nice too, though again perhaps not required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a test for empty labels. I am not sure which behaviour we want:

  1. an error "Dimensions of Z and labels must be consistent." (this is the current behaviour)
  2. labels=[] is the same as labels=None (labels=None is the default)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also added # test for gh-12418.

In general I feel git blame works reasonably well to find the associated PR with the discussion. At the same time it may be annoying if the history is complicated so I can see why the comment is useful.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer an error, as it is now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK great, let me know if you need anything else, maybe a changelog entry somewhere?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is it, our changelog for the release notes will be auto-generated.

@lesteve
Copy link
Contributor Author

lesteve commented Jun 25, 2020

LGTM. I'm sorry my previous PR did not test ndarray.

Not a big deal at all. IMO the strength of an ecosystem is not that its members never make any mistake but instead that people are motivated to work together and fix others mistakes when they bump into it and overall make the ecosystem a better place to live in.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

LGTM, merging. Thanks @lesteve, all

@rgommers rgommers merged commit 4bf5b9f into scipy:master Jun 25, 2020
@lesteve lesteve deleted the fix-dendrogram-regression branch June 25, 2020 09:12
@tylerjereddy tylerjereddy modified the milestones: 1.6.0, 1.5.1 Jul 4, 2020
tylerjereddy pushed a commit to tylerjereddy/scipy that referenced this pull request Jul 4, 2020
@tylerjereddy tylerjereddy removed the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Jul 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.cluster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in hierarchy.dendogram
4 participants