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.

Sign up for GitHub

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

Jump to bottom

Add variate transport #62

Merged
merged 70 commits into from
Jun 19, 2022
Merged

Add variate transport #62

merged 70 commits into from
Jun 19, 2022

Conversation

oschulz
Copy link
Collaborator

@oschulz oschulz commented Jun 15, 2022

No description provided.

@oschulz oschulz requested a review from cscherrer June 15, 2022 14:06
@oschulz oschulz marked this pull request as draft June 15, 2022 14:06
@github-actions
Copy link

Package name latest stable
MeasureTheory.jl

@github-actions
Copy link

Package name latest stable
MeasureTheory.jl

@codecov
Copy link

codecov bot commented Jun 15, 2022 •
edited
Loading

Codecov Report

Merging #62 (a8faa66) into master (bf7eae6) will increase coverage by 8.53%.
The diff coverage is 78.57%.

@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
+ Coverage   39.02%   47.55%   +8.53%     
==========================================
  Files          34       37       +3     
  Lines         756      942     +186     
==========================================
+ Hits          295      448     +153     
- Misses        461      494      +33
Impacted Files Coverage Δ
src/MeasureBase.jl 60.00% <ø> (ø)
src/insupport.jl 0.00% <0.00%> (ø)
src/transport.jl 63.88% <63.88%> (ø)
src/combinators/transformedmeasure.jl 71.42% <80.64%> (+71.42%) ⬆️
src/getdof.jl 87.50% <87.50%> (ø)
src/interface.jl 96.29% <94.11%> (-3.71%) ⬇️
src/combinators/power.jl 84.90% <100.00%> (+12.40%) ⬆️
src/combinators/weighted.jl 81.81% <100.00%> (+2.87%) ⬆️
src/primitives/dirac.jl 70.58% <100.00%> (+9.04%) ⬆️
src/primitives/lebesgue.jl 80.95% <100.00%> (+4.48%) ⬆️
... and 5 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 bf7eae6...a8faa66. Read the comment docs.

cscherrer
cscherrer reviewed Jun 15, 2022
View reviewed changes
Copy link
Collaborator

@cscherrer cscherrer left a comment

Choose a reason for hiding this comment

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

Thanks @oschulz ! I have some questions, may have a better basis for review after I understand some things better

src/effndof.jl Outdated Show resolved Hide resolved
src/effndof.jl Outdated Show resolved Hide resolved
src/effndof.jl Outdated Show resolved Hide resolved
src/insupport.jl Outdated Show resolved Hide resolved
src/vartransform.jl Outdated Show resolved Hide resolved
src/vartransform.jl Outdated Show resolved Hide resolved
src/vartransform.jl Outdated Show resolved Hide resolved
src/vartransform.jl Outdated Show resolved Hide resolved
src/vartransform.jl Outdated Show resolved Hide resolved
@github-actions
Copy link

Package name latest stable
MeasureTheory.jl

@github-actions
Copy link

Package name latest stable
MeasureTheory.jl

@github-actions
Copy link

Package name latest stable
MeasureTheory.jl

@github-actions
Copy link

Package name latest stable
MeasureTheory.jl

@github-actions
Copy link

Package name latest stable
MeasureTheory.jl

@oschulz
Copy link
Collaborator Author

oschulz commented Jun 19, 2022 •
edited
Loading

If we want to stress the transport aspect, I'd suggest transport_map(nu, mu) or gettransport(nu, mu) (with transport_def(nu, mu, x) for the internal function, and nu the target and mu the source).

@oschulz
Copy link
Collaborator Author

oschulz commented Jun 19, 2022

Ok, new list for bikeshedding:

  • transformation_to(nu, mu)
  • transformation_map(nu, mu)
  • transport_to(nu, mu)
  • transport_map(nu, mu)

I like the ones with _to better, but I'm Ok with all of them.

@cscherrer , @mschauer , any picks?

@cscherrer
Copy link
Collaborator

I think it's important to try to distinguish between a measure transformation and a support transformation (that can be pushed forward or pulled back for some measure). And the to/from thing could be confusing.

If the concept of a transport is meaningful when the spaces are different, I like transport_to

@oschulz
Copy link
Collaborator Author

oschulz commented Jun 19, 2022

I think it's important to try to distinguish between a measure transformation and a support transformation (that can be pushed forward or pulled back for some measure). And the to/from thing could be confusing.

If the concept of a transport is meaningful when the spaces are different, I like transport_to

If I read https://en.m.wikipedia.org/wiki/Transportation_theory_(mathematics) correctly, the spaces can be different. Correct, @mschauer?

We do not guarantee that we return an optimal transport, though (we shovel the earth correctly but not necessarily with minimum effort).

@mschauer
Copy link
Member

Exactly, we also don’t promise an optimal transport, right? :-)

@oschulz
Copy link
Collaborator Author

oschulz commented Jun 19, 2022

Exactly, we also don’t promise an optimal transport, right? :-)

Indeed no, just a "correct transport, not an optimal one.

@cscherrer
Copy link
Collaborator

@mschauer if we change to transport_to do you think we're good to go?

I'm going to be out for a while, so once you're both happy with this please go ahead and merge, and tag a release

@mschauer
Copy link
Member

I should not tag releases on the phone but I am happy with the PR

@oschulz
Copy link
Collaborator Author

oschulz commented Jun 19, 2022

I should not tag releases on the phone but I am happy with the PR

Ok, let me do the rename, then we'll tag?

@oschulz
Copy link
Collaborator Author

oschulz commented Jun 19, 2022

Ok, renamed, let's see if CI goes through.

@mschauer Ok like this?

@github-actions
Copy link

Package name latest stable
MeasureTheory.jl

@oschulz
Copy link
Collaborator Author

oschulz commented Jun 19, 2022

Ah, stop, need to rename vartransform_origin to

@oschulz
Copy link
Collaborator Author

oschulz commented Jun 19, 2022

Ok rename complete.

@github-actions
Copy link

Package name latest stable
MeasureTheory.jl

@oschulz
Copy link
Collaborator Author

oschulz commented Jun 19, 2022

I should not tag releases on the phone but I am happy with the PR

I can do the squash-merge, increase version number and tag, if there are no objections?

@oschulz
Copy link
Collaborator Author

oschulz commented Jun 19, 2022 •
edited
Loading

This will be v0.11.0, not v0.10.1, right?

@oschulz
Copy link
Collaborator Author

oschulz commented Jun 19, 2022

This will be v0.11.0, not v0.10.1, right?

Ah, yes - has to be, since it moves out StdNormal.

@github-actions
Copy link

Package name latest stable
MeasureTheory.jl

@oschulz oschulz merged commit 648376d into JuliaMath:master Jun 19, 2022
@oschulz oschulz deleted the transforms branch June 19, 2022 21:27
@oschulz
Copy link
Collaborator Author

oschulz commented Jun 19, 2022

Released. :-)

@oschulz oschulz changed the title Add vartransform Add variate transport Jun 20, 2022
@oschulz
Copy link
Collaborator Author

oschulz commented Jun 20, 2022 •
edited
Loading

I apologize, I overlooked one name - we still have the VarTransform type. Should I make a v0.12 (we could name it to TransportFunction? It's not a type users will interact with much, but maybe best to fix that naming inconsistency now instead of later?

@oschulz
Copy link
Collaborator Author

oschulz commented Jun 20, 2022

I did a grep for var ... we also forgot about test_vartransform, NoTransport, checked_var, NoVarCheck and a few internal functions during the rename (oops).

@oschulz oschulz mentioned this pull request Jun 20, 2022
Merged
@oschulz
Copy link
Collaborator Author

oschulz commented Jun 20, 2022

#63 should fix it - grep shows no remaining "...var..." where it doesn't belong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mschauer mschauer mschauer approved these changes

@cscherrer cscherrer cscherrer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@oschulz @cscherrer @mschauer