Skip to content

Conversation

brahmaneya
Copy link
Collaborator

@brahmaneya brahmaneya commented Jan 27, 2020

#Description of proposed changes
Squeezing a length 1 array for used to create a scalar instead of a 1D array as intended. Fixed by adding an extra check.
Also made munkres version in setup.py consistent with requirements.

Test plan

Update unit test for changed function.

Checklist

Need help on these? Just ask!

  • I have read the CONTRIBUTING document.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • I have run tox -e complex and/or tox -e spark if appropriate.
  • All new and existing tests passed.

Copy link
Member

@vincentschen vincentschen left a comment

Choose a reason for hiding this comment

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

nice! why are the munkres changes related?

include_package_data=True,
install_requires=[
"munkres==1.1.2",
"munkres>=1.0.6",
Copy link
Member

Choose a reason for hiding this comment

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

are these version commits related to this patch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They're not, but tox -e check was failing without it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll try reverting it and see if travis ci passes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CI fails if I revert.

Comment on lines +24 to +27
Y = to_int_label_array(np.array([[1]]), flatten_vector=True)
Y_expected = np.array([1])
np.testing.assert_array_equal(Y, Y_expected)

Copy link
Member

Choose a reason for hiding this comment

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

nice!

@codecov
Copy link

codecov bot commented Jan 27, 2020

Codecov Report

Merging #1540 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1540      +/-   ##
==========================================
+ Coverage   97.11%   97.11%   +<.01%     
==========================================
  Files          55       55              
  Lines        2077     2079       +2     
  Branches      341      342       +1     
==========================================
+ Hits         2017     2019       +2     
  Misses         31       31              
  Partials       29       29
Impacted Files Coverage Δ
snorkel/utils/core.py 94.44% <100%> (+0.21%) ⬆️

Copy link
Member

@vincentschen vincentschen left a comment

Choose a reason for hiding this comment

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

nice catch!! lgtm 🚢

@brahmaneya brahmaneya merged commit 88b2579 into master Jan 27, 2020
@brahmaneya brahmaneya deleted the squeezefix branch January 27, 2020 19:26
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 this pull request may close these issues.

2 participants