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

Amplitude arrays incorrectly filled in diagrams.Amplitude #79

Closed
ulupo opened this issue Nov 5, 2019 · 0 comments
Closed

Amplitude arrays incorrectly filled in diagrams.Amplitude #79

ulupo opened this issue Nov 5, 2019 · 0 comments
Assignees
Labels
bug Something isn't working

Comments

@ulupo
Copy link
Collaborator

ulupo commented Nov 5, 2019

Description

The transform method in diagrams.Amplitude calls the _parallel_pairwise utility function for joblib-parallel computation of amplitudes of persistence diagrams. This function first calculates all amplitudes for different homology dimensions and across different slices of the input array of diagrams:

https://github.com/giotto-learn/giotto-learn/blob/a5f2024375fd24c84614b9b77ffc39e79cb71cac/giotto/diagrams/_metrics.py#L204

Then, we have to carefully arrange all amplitudes into an array of the correct shape:

https://github.com/giotto-learn/giotto-learn/blob/a5f2024375fd24c84614b9b77ffc39e79cb71cac/giotto/diagrams/_metrics.py#L210

Unfortunately, the final result at present is incorrect, primarily because the top for in the call to Parallel is on the slices, while it should be on the list of homology dimensions. Changing the order of the for loops as well as replacing the final line before return with

amplitude_arrays = np.concatenate(amplitude_arrays).reshape(len(homology_dimensions), X.shape[0]).T

woud yield correct results.

Steps/Code to Reproduce

Create

diagrams = np.array([
    [[0, 1, 0.],
     [0, 0, 0.],
     [0, 4, 1.]],  # Expected bottleneck ampl: [sqrt(2)/2, 2*sqrt(2)] 
    
    [[0, 2, 0.],
     [0, 1, 0.],
     [0, 0, 1.]],  # Expected bottleneck ampl: [sqrt(2), 0] 
    
    [[3, 3.5, 0.],
     [0, 0, 0.],
     [5, 9, 1.]]  # Expected bottleneck ampl: [0.5*sqrt(2)/2, 2*sqrt(2)] 
])

Expected Results

The expected amplitude array when using bottleneck amplitude is

array([[0.70710678, 2.82842712],
       [1.41421356, 0.        ],
       [0.35355339, 2.82842712]])

Actual Results

The current result is

array([[0.70710678, 1.41421356],
       [2.82842712, 0.        ],
       [0.35355339, 2.82842712]])

Versions

giotto-learn: 0.1.2

@ulupo ulupo added the bug Something isn't working label Nov 5, 2019
ulupo pushed a commit to ulupo/giotto-tda that referenced this issue Nov 5, 2019
@ulupo ulupo mentioned this issue Nov 5, 2019
gtauzin pushed a commit that referenced this issue Nov 6, 2019
@gtauzin gtauzin closed this as completed Nov 6, 2019
ulupo pushed a commit to ulupo/giotto-tda that referenced this issue Nov 7, 2019
gtauzin pushed a commit that referenced this issue Nov 7, 2019
* Fix (#79)

* Create test to avoid (#79) in future
gtauzin pushed a commit that referenced this issue Nov 7, 2019
* Update azure-pipelines.yml (#36) (#56)

* Update azure-pipelines.yml

* Update azure-pipelines.yml

* Update azure-pipelines.yml

* Fixed typos and added joblib (#60)

* Update CONTRIBUTING.rst

* Fix (#66) (#67)

* Remove "www" from theory links, cite ripser.py and dyonisus 2, add references to Perea and Bauer

* Update CODE_OF_CONDUCT.rst (#64)

* Update the CLA signing process using https://cla-assistant.io.

* Fix openml id (#69) (#70)

* Fix openml id in Lorenz_Attractor.ipynb

* Add python 35 support (#61) (#63)

* Update azure-pipelines.yml

* Update plotting.py

* Update validation.py

* Update H2_on_the_plane.ipynb

* Update setup.py with Python>=3.5 (#63)

* Formatting fix

* Typo fix in URL

* Small URL fix in README

* Fix typo (#76)

* Update _version.py for version 0.1.2.

* Fix (#79) (#80)

* Update README.rst with new account name.

* Bindings update (#81)

* Add clang format files following Google Guidelines for C++

Signed-off-by: julian <julian.burellaperez@heig-vd.ch>

* Update code format of all C++ bindings

Signed-off-by: julian <julian.burellaperez@heig-vd.ch>

* Update cubical complex include, to add the library form the correct path

Update CMakeLists.txt file, to indicate the path to the Gudhi's interface

Signed-off-by: julian <julian.burellaperez@heig-vd.ch>

* Update persistent cohomology, to add the library from the correct location

Update CMakeLists.txt file, to indicate the path to the Gudhi's interface
Delete old local interface file

Signed-off-by: julian <julian.burellaperez@heig-vd.ch>

* Update simplex tree include, to add the library from the correct location

Update CMakeLists.txt file, to indicate the path to the Gudhi's interface
Delete old local interface file
Fix unused include in rips_complex which made build fail

Signed-off-by: julian <julian.burellaperez@heig-vd.ch>

* Update headers of bindings:

Remove author name
Update license

Signed-off-by: julian <julian.burellaperez@heig-vd.ch>

* Update more headers of bindings:

Remove author name
Update license

Signed-off-by: julian <julian.burellaperez@heig-vd.ch>

* update submodule commit

Signed-off-by: julian <julian.burellaperez@heig-vd.ch>

* Add test to avoid (#79) (#83)

* Fix (#79)

* Create test to avoid (#79) in future

* Update _version.py for v0.1.3

* Update setup.py for v0.1.3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants