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

tentative of random subset for random matrix #36313

Merged
merged 2 commits into from
Oct 14, 2023

Conversation

fchapoton
Copy link
Contributor

trying to do something for #35664 ; not yet good

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

use_complement = False

subset = []
available = list(range(n))
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be better to shuffle the list available and then to return the first k items (or complement n - k items).

Alternatively, if you prefer successive calls to randint, then instead of calling available.pop(index), you can simply do available[index] = available[-1].

@fchapoton
Copy link
Contributor Author

bonne idée, merci. Mais je soupconne qu'il reste un probleme ailleurs, dans la procedure qui utilise la nouvelle fonction

src/sage/matrix/special.py Outdated Show resolved Hide resolved
"""
Return a random subset of range(n) of size k.

The distribution law is uniform.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you use uniform distribution but the original method used a beta distribution with parameters [1.6, 4.3]. ..

src/sage/matrix/special.py Outdated Show resolved Hide resolved
@fchapoton
Copy link
Contributor Author

y a un bug avec l'usage de "enumerate"

@dcoudert
Copy link
Contributor

Je ne vois vraiment pas pourquoi. C'est vraiment lié à ça ?

@github-actions
Copy link

github-actions bot commented Oct 3, 2023

Documentation preview for this PR (built with commit 1d36521; changes) is ready! 🎉

@fchapoton
Copy link
Contributor Author

faut encore attendre la fin des tests

@dcoudert
Copy link
Contributor

dcoudert commented Oct 3, 2023

Something goes wrong

sage -t --long --random-seed=286735480429121101562228604801325644303 sage/modular/local_comp/liftings.py
**********************************************************************
File "sage/modular/local_comp/liftings.py", line 244, in sage.modular.local_comp.liftings.lift_for_SL
Failed example:
    for _ in range(100):
        d = randint(0, 10)
        p = choice([2,3,5,7,11])
        M = random_matrix(Zmod(p), d, algorithm='unimodular')
        assert lift_for_SL(M).det() == 1
Exception raised:
    Traceback (most recent call last):
      File "/sage/src/sage/doctest/forker.py", line 709, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/sage/src/sage/doctest/forker.py", line 1144, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.modular.local_comp.liftings.lift_for_SL[19]>", line 4, in <module>
        M = random_matrix(Zmod(p), d, algorithm='unimodular')
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/sage/src/sage/matrix/special.py", line 666, in random_matrix
        return random_unimodular_matrix(parent, *args, **kwds)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/sage/src/sage/matrix/special.py", line 3037, in random_unimodular_matrix
        return random_matrix(ring, size, algorithm='echelonizable', rank=size)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/sage/src/sage/matrix/special.py", line 660, in random_matrix
        return random_echelonizable_matrix(parent, *args, **kwds)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/sage/src/sage/matrix/special.py", line 2713, in random_echelonizable_matrix
        matrix = random_rref_matrix(parent, rank)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/sage/src/sage/matrix/special.py", line 2569, in random_rref_matrix
        for rest_non_pivot_column in range(pivots[num_pivots-1]+1,num_col):
                                           ~~~~~~^^^^^^^^^^^^^^
    IndexError: list index out of range

@fchapoton
Copy link
Contributor Author

this is a problem in size 0 x 0

@fchapoton
Copy link
Contributor Author

Ca devrait etre ok maintenant.

src/sage/matrix/special.py Outdated Show resolved Hide resolved
src/sage/matrix/special.py Outdated Show resolved Hide resolved
return_matrix = copy(parent.zero_matrix())

# No harm if no pivots at all.
subset = list(range(1, num_col))
Copy link
Contributor

Choose a reason for hiding this comment

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

to be on the safe side, you may add a test for num_col > 1 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bah, ca me parait pas indispensable

Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

Parfait.

LGTM.

@vbraun vbraun merged commit 1943799 into sagemath:develop Oct 14, 2023
20 of 22 checks passed
@mkoeppe mkoeppe added this to the sage-10.2 milestone Oct 14, 2023
@fchapoton fchapoton deleted the random_subset_etc branch October 15, 2023 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants