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

Extension of spatial.distance module #479

Merged
merged 9 commits into from
Feb 11, 2020

Conversation

Cdebus
Copy link
Contributor

@Cdebus Cdebus commented Feb 7, 2020

  • refactorization to scipy naming
  • restructuring of distance calculation logic
  • support for distance calculation of two different tensors with different splits
  • support for dtypes other than float32

Description

Issue/s resolved:
Addresses #475 amongst other changes

Changes proposed:

  • Implementation of abstract function _dist(X,Y, metric)
  • derive cdist and rbf as explicit implementatiions with metrices euclidian and gaussian

Type of change

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

Due Diligence

  • All split configurations tested
  • Multiple dtypes tested in relavent 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

- refactorization to scipy naming
- restructuring of distance calculation logic
- support for distance calculation of two different tensors with different splits
- support for dtypes other than float32
@Cdebus Cdebus marked this pull request as ready for review February 7, 2020 15:39
@codecov
Copy link

codecov bot commented Feb 7, 2020

Codecov Report

Merging #479 into master will increase coverage by 0.03%.
The diff coverage is 98.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #479      +/-   ##
==========================================
+ Coverage   96.72%   96.76%   +0.03%     
==========================================
  Files          60       60              
  Lines       12281    12562     +281     
==========================================
+ Hits        11879    12155     +276     
- Misses        402      407       +5
Impacted Files Coverage Δ
heat/cluster/kmeans.py 82.22% <100%> (ø) ⬆️
heat/spatial/__init__.py 100% <100%> (ø) ⬆️
heat/spatial/tests/test_distances.py 96.96% <100%> (+6.77%) ⬆️
heat/spatial/distance.py 97.34% <97.34%> (ø)

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 29be6aa...a65b5cc. Read the comment docs.

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.

looks pretty good. needs a few small updates but good job!

heat/spatial/distance.py Outdated Show resolved Hide resolved
stat = MPI.Status()
comm.handle.Probe(source=sender, tag=iter, status=stat)
count = int(stat.Get_count(mpi_type) / f)
moving = torch.zeros((count, f), dtype=torch_type)
Copy link
Member

Choose a reason for hiding this comment

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

device argument

heat/spatial/distance.py Outdated Show resolved Hide resolved
heat/spatial/distance.py Show resolved Hide resolved
heat/spatial/distance.py Outdated Show resolved Hide resolved
heat/spatial/distance.py Show resolved Hide resolved
@coquelin77
Copy link
Member

coquelin77 commented Feb 10, 2020

would it be possible to abstract the communications loop that you use in _dist for the circular sending/receiving?

@Cdebus
Copy link
Contributor Author

Cdebus commented Feb 11, 2020

would it be possible to abstract the communications loop that you use in _dist for the circular sending/receiving?

I don't know, in principle yes, but what would be the application. The key point here ist the calculation of metrics between chunks from X and Y. While this is easily substituted, it can already currently be modified by the function you pass as metric.

@coquelin77 coquelin77 merged commit 7a907f7 into helmholtz-analytics:master Feb 11, 2020
@Cdebus Cdebus deleted the features/distance branch April 7, 2020 11:06
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