geomstats / challenge-iclr-2021

GitHub repository for the ICLR Computational Geometry & Topology Challenge 2021
MIT License
51 stars 25 forks source link

Matrices "belongs" function is not giving the expected result on non-square matrices #9

Closed elodiemaignant closed 3 years ago

elodiemaignant commented 3 years ago

I think it has to do with the use of "&" logic operator rather than "and" one.

import geomstats.backend as gs
import geomstats.datasets.utils as data_utils
from geomstats.geometry.matrices import Matrices

M = Matrices(3,3)

N = gs.zeros((3,3))
print(M.belongs(N))

M = Matrices(3,2)

N = gs.zeros((3,2))
print(M.belongs(N))
ninamiolane commented 3 years ago

Hi @emaignant , thank you for submitting this issue! As a participant to the challenge, feel free to mention the issues you have submitted as part of your notebook within the section "Describe the limitations of the packages Geomstats and Giotto-tda".

Which python version and geomstats version do you use? It was working in my environment for python 3.6, 3.7, 3.8 and 3.9, I was not able to reproduce the bug.

I did replace the & by and in the codebase, as you suggested, and added unit tests on geomstats - see this PR: https://github.com/geomstats/geomstats/pull/942

Let me know if this solves the issue on your side? Note that you will have to pull geomstats' master branch from GitHub, geomstats PyPi version will be updated in a few days only.

nguigs commented 3 years ago

For the record: https://stackoverflow.com/questions/22646463/and-boolean-vs-bitwise-why-difference-in-behavior-with-lists-vs-nump

ninamiolane commented 3 years ago

@emaignant was your issue solved?

ninamiolane commented 3 years ago

Solved, as per slackcommunications with @emaignant