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

Finite Discrete Measure #95

Merged

Conversation

davibarreira
Copy link
Member

@davibarreira davibarreira commented Jun 4, 2021

This PR has the implementation of the FiniteDiscreteMesure struct which I talked about in #92. It is similar to the DiscreteNonParametric type in Distributions.jl. It contains a supportand a p (for probability), allowing the user to store the empirical distributions. This would allow use to write functions such as sinkhorn2(c, mu::FiniteDiscreteMeasure, nu::FiniteDiscreteMeasure), without having to pass both support and probabilities separately.

p.s: I don't feel strongly about the FiniteDiscreteMeasure naming. Feel free to give better suggestions. I just don't think DiscreteMeasure is a good name, because our variable cannot have infinite support, like a Poisson distribution.
SimpleMeasure is another option and would be theoretically correct. EmpiricalMeasureand SampleMeasure are another option, but they are kind of ambiguous, since the measure does not need necessarily to be either empirical or from a sampling process.

@coveralls
Copy link

coveralls commented Jun 4, 2021

Pull Request Test Coverage Report for Build 914791279

  • 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 94.672%

Totals Coverage Status
Change from base Build 911721124: 0.1%
Covered Lines: 462
Relevant Lines: 488

💛 - Coveralls

@codecov-commenter
Copy link

codecov-commenter commented Jun 4, 2021

Codecov Report

Merging #95 (d5d57c7) into master (24ecf5d) will decrease coverage by 0.21%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #95      +/-   ##
==========================================
- Coverage   94.88%   94.67%   -0.22%     
==========================================
  Files           6        7       +1     
  Lines         430      488      +58     
==========================================
+ Hits          408      462      +54     
- Misses         22       26       +4     
Impacted Files Coverage Δ
src/utils.jl 100.00% <100.00%> (ø)
src/entropic.jl
src/entropic/sinkhorn.jl 97.98% <0.00%> (ø)
src/entropic/sinkhorn_stabilized.jl 94.33% <0.00%> (ø)

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 24ecf5d...d5d57c7. Read the comment docs.

src/utils.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
src/OptimalTransport.jl Outdated Show resolved Hide resolved
src/OptimalTransport.jl Outdated Show resolved Hide resolved
src/OptimalTransport.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
davibarreira and others added 6 commits June 4, 2021 19:13
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
@davibarreira
Copy link
Member Author

There is some compatibility issue, but I cannot reproduce in my machine. It seems to be related to KernelFunctions, which I used in the test.

@devmotion
Copy link
Member

KernelFunctions requires Julia >= 1.3 (this is one reason for why we want to extract ColVecs and RowVecs to a separate package). It seems ArraysOfArrays also supports Julia 1.0, so it would be better to use it in the tests until ColVecs and RowVecs are compatible with Julia 1.0.

@davibarreira
Copy link
Member Author

If there are no more proposed changes, I think this is ready to be merged :D

docs/src/index.md Outdated Show resolved Hide resolved
src/OptimalTransport.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
test/utils.jl Outdated Show resolved Hide resolved
test/utils.jl Outdated Show resolved Hide resolved
test/utils.jl Outdated Show resolved Hide resolved
davibarreira and others added 7 commits June 6, 2021 11:32
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
test/utils.jl Outdated Show resolved Hide resolved
test/utils.jl Outdated Show resolved Hide resolved
test/utils.jl Outdated Show resolved Hide resolved
davibarreira and others added 3 commits June 6, 2021 14:13
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

OK, let's merge this and see how we can make use of it. Since it's not exported we can iterate freely until we reach a good design 🙂

@devmotion devmotion merged commit 93c6077 into JuliaOptimalTransport:master Jun 7, 2021
@davibarreira
Copy link
Member Author

KernelFunctions requires Julia >= 1.3 (this is one reason for why we want to extract ColVecs and RowVecs to a separate package). It seems ArraysOfArrays also supports Julia 1.0, so it would be better to use it in the tests until ColVecs and RowVecs are compatible with Julia 1.0.

@devmotion would you know if the ColVecs and RowVecs have been moved to another package? I'm writing some extra documentation on the 1D case, and I remembered this.

@coveralls
Copy link

coveralls commented Sep 26, 2024

Pull Request Test Coverage Report for Build 911924330

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 94.672%

Totals Coverage Status
Change from base Build 910296728: -0.2%
Covered Lines: 462
Relevant Lines: 488

💛 - Coveralls

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.

4 participants