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

diag() and diagonal() feature #412

Merged
merged 10 commits into from
Dec 6, 2019
Merged

diag() and diagonal() feature #412

merged 10 commits into from
Dec 6, 2019

Conversation

TheSlimvReal
Copy link
Contributor

@TheSlimvReal TheSlimvReal commented Nov 14, 2019

Description

This PR targets issue #242.

The returned array of diag() in case of creating a diagonal array, will be split the same as the input array (split=None or split=0).

Type of change

Select relevant options.

  • New feature (non-breaking change which adds functionality)

Are all split configurations tested and accounted for?
[x] yes - [ ] no
Does this change require a documentation update outside of the changes proposed?
[ ] yes [x] no
Does this change modify the behaviour of other functions?
[ ] yes [x] no
Are there code practices which require justification?
[ ] yes [x] no

@codecov
Copy link

codecov bot commented Nov 20, 2019

Codecov Report

Merging #412 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #412      +/-   ##
==========================================
+ Coverage   98.02%   98.05%   +0.03%     
==========================================
  Files          55       55              
  Lines       11066    11285     +219     
==========================================
+ Hits        10847    11066     +219     
  Misses        219      219
Impacted Files Coverage Δ
heat/core/tests/test_manipulations.py 100% <100%> (ø) ⬆️
heat/core/manipulations.py 99.2% <100%> (+0.1%) ⬆️

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 3e7c397...f28b0b0. Read the comment docs.

@TheSlimvReal
Copy link
Contributor Author

I also made some small changes in the BasicTest file because this was the first time I used it in production. I hope it's okay to push this changes together.

Copy link
Member

@coquelin77 coquelin77 left a comment

Choose a reason for hiding this comment

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

the logic here looks good, only minor things to change.

heat/core/manipulations.py Outdated Show resolved Hide resolved
heat/core/manipulations.py Outdated Show resolved Hide resolved
heat/core/manipulations.py Outdated Show resolved Hide resolved
heat/core/manipulations.py Outdated Show resolved Hide resolved
heat/core/manipulations.py Outdated Show resolved Hide resolved
heat/core/tests/test_manipulations.py Show resolved Hide resolved
heat/core/tests/test_manipulations.py Show resolved Hide resolved
@coquelin77
Copy link
Member

tests failing, bump

Copy link
Contributor Author

@TheSlimvReal TheSlimvReal left a comment

Choose a reason for hiding this comment

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

I merge the branch #420 into this to get the correctly working unit test.
Please merge that branch before this one.

@coquelin77 coquelin77 merged commit 84587e3 into master Dec 6, 2019
@coquelin77 coquelin77 deleted the feature/242-diag branch December 6, 2019 15:02
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