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

Features/538 larray #680

Merged
merged 10 commits into from
Oct 19, 2020
Merged

Features/538 larray #680

merged 10 commits into from
Oct 19, 2020

Conversation

lenablind
Copy link
Collaborator

@lenablind lenablind commented Oct 6, 2020

Description

Introduction of property larray, which allows access to the underlying local torch tensor (_DNDarray__array / __array attribute of DNDarray).
For the setter, I included a warning as requested in the issue.

Consequently, I replaced all usages of _DNDarray__array within the project space with larray.

As I specified torch.tensor as the only valid dtype for this attribute, I had to adapt function average in statistics, simply wrapping a float assignment into a tensor containing that specific float (just one line (328) modified).

Issue/s resolved: #538

Changes proposed:

  • Introduction of property larray in dndarray
  • replacement of all usages of _DNDarray__array with larray within the project space
  • Corresponding tests (especially for the setter)
  • Minimal adaption of function average in statistics (line 328)

Type of change

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

Due Diligence

  • All split configurations tested
  • Multiple dtypes tested in relevant functions
  • Documentation updated (if needed)
  • Updated changelog.md under the title "Pending Additions"

Does this change modify the behaviour of other functions? If so, which?

no (, not after the presented modification of average in statistics. Without the wrapping, a TypeError is thrown as specified in the setter of larray, which expected a torch.tensor but got a float (in the corresponding tests) instead).

@codecov
Copy link

codecov bot commented Oct 6, 2020

Codecov Report

Merging #680 into master will increase coverage by 0.00%.
The diff coverage is 98.12%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #680   +/-   ##
=======================================
  Coverage   97.47%   97.47%           
=======================================
  Files          87       87           
  Lines       17182    17231   +49     
=======================================
+ Hits        16748    16796   +48     
- Misses        434      435    +1     
Impacted Files Coverage Δ
heat/core/tests/test_communication.py 97.96% <ø> (ø)
heat/core/linalg/solver.py 68.36% <25.00%> (ø)
heat/naive_bayes/gaussianNB.py 93.50% <66.66%> (ø)
heat/core/io.py 90.32% <75.00%> (ø)
heat/core/rounding.py 95.08% <80.00%> (ø)
heat/core/dndarray.py 96.66% <88.88%> (-0.08%) ⬇️
heat/core/communication.py 90.35% <90.90%> (ø)
heat/core/linalg/basics.py 94.46% <95.23%> (ø)
heat/cluster/kmedians.py 70.58% <100.00%> (ø)
heat/cluster/kmedoids.py 76.19% <100.00%> (ø)
... and 36 more

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 bc99e60...3d02270. Read the comment docs.

@Markus-Goetz
Copy link
Member

Outstanding job Lena. As far as I can see, our core is now issuing a lot of warning. As per issue that was asked for. However, I feel, that I should modify this as to: if the user is using the larray setter a warning should be issued, if the core is using larray, then warnings should be suppressed, as we are supposedly know what we are doing. Do you think you can change it to be that way? Or do you have differing thoughts?

@lenablind
Copy link
Collaborator Author

Outstanding job Lena. As far as I can see, our core is now issuing a lot of warning. As per issue that was asked for. However, I feel, that I should modify this as to: if the user is using the larray setter a warning should be issued, if the core is using larray, then warnings should be suppressed, as we are supposedly know what we are doing. Do you think you can change it to be that way? Or do you have differing thoughts?

Thank you, Markus. Yes, I agree, this sounds reasonable to me. I'll try to change it to be that way.

coquelin77
coquelin77 previously approved these changes Oct 13, 2020
@coquelin77
Copy link
Member

@lenablind can you handle the merge here? there is some minor conflicts with the nbytes feature

@lenablind
Copy link
Collaborator Author

@lenablind can you handle the merge here? there is some minor conflicts with the nbytes feature

@coquelin77 Yes for sure! Thank you for taking care of this and the previous PRs

@coquelin77 coquelin77 merged commit 5cd2b42 into master Oct 19, 2020
@coquelin77 coquelin77 deleted the features/538-larray branch October 19, 2020 08:23
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.

Introduce larray property
3 participants