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

Fix all three CONDA ci failing tests #38649

Merged
merged 5 commits into from
Sep 22, 2024

Conversation

fchapoton
Copy link
Contributor

@fchapoton fchapoton commented Sep 11, 2024

just some details in the code and tests

including one fix for conda runs, that should not test sympy behaviour

and also fixing 2 other conda-related failures

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

@fchapoton fchapoton added s: run conda ci Run the conda workflow on this PR. and removed s: needs review labels Sep 11, 2024
Copy link

github-actions bot commented Sep 11, 2024

Documentation preview for this PR (built with commit 7607de8; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@fchapoton
Copy link
Contributor Author

now also trying to fix the other 2 conda failures

@fchapoton fchapoton removed the s: run conda ci Run the conda workflow on this PR. label Sep 11, 2024
@fchapoton fchapoton changed the title Fix hypergeometric Fix all three CONDA ci failing tests Sep 12, 2024
@fchapoton fchapoton added p: CI Fix merged before running CI tests and removed p: CI Fix merged before running CI tests labels Sep 12, 2024
@fchapoton
Copy link
Contributor Author

who can set the CI-fix label ? after or before positive review ?

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 12, 2024

If it is a hot fix for ci, ok before review.

@fchapoton
Copy link
Contributor Author

ok, I will add the label. So please review

@fchapoton fchapoton added the p: CI Fix merged before running CI tests label Sep 13, 2024
@dcoudert
Copy link
Contributor

The time complexity of current version of eliminate_parameters is in $O(|a|*|b| + |a|^2 + |b|^2)$, due to the time needed to check if a parameter is in a list (linear in the length of the list) and to remove it from a list (linear in the length of the list).
We can have $O(|a| + |b|)$ as follows:

def eliminate_parameters(self, a, b, z):
    r"""
    Eliminate repeated parameters by pairwise cancellation of identical
    terms in ``a`` and ``b``.

    If `p` is in `a` and in `b`, then remove the first occurence of `p` in both.
    """
    # Record the positions of the parameters in a
    pos_in_a = {}
    for pos, p in enumerate(a):
        if p in pos_in_a:
            pos_in_a[p].append(pos)
        else:
            pos_in_a[p] = [pos]
    # Reverse the lists of positions for faster access to first remaining
    # occurence of each parameter
    for v in pos_in_a.values():
        v.reverse()

    # If a parameter is in both a and b, remove its first occurence
    new_b = []
    keep_in_a = [True] * len(a)
    for p in b:
        if p in pos_in_a and pos_in_a[p]:
            keep_in_a[pos_in_a[p].pop()] = False
        else:
            new_b.append(p)

    if len(b) == len(new_b):
        return self

    new_a = [p for p, k in zip(a, keep_in_a) if k]

    return hypergeometric(new_a, new_b, z)

@fchapoton
Copy link
Contributor Author

good idea, but rather elsewhere please, as this PR is also used as a fix for Conda CI

@fchapoton
Copy link
Contributor Author

note also that probably nobody uses hypergeometric functions with more than 10 arguments..

@dcoudert
Copy link
Contributor

it seems interesting even for a small number of parameters. I agree it's a small improvement

sage: a, b = [1, 1, 2, 5], [5, 1, 4]
sage: H = hypergeometric(a, b, 1/2)
sage: H.eliminate_parameters()
hypergeometric((1, 2), (4,), 1/2)
sage: eliminate_parameters(H, a, b, 1/2)
hypergeometric((1, 2), (4,), 1/2)
sage: %timeit H.eliminate_parameters()
16.3 µs ± 75 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
sage: %timeit eliminate_parameters(H, a, b, 1/2)
13.8 µs ± 45.7 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

@fchapoton
Copy link
Contributor Author

would please someone give a positive review ?

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.

LGTM.

vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 19, 2024
    
just some details in the code and tests

including one fix for conda runs, that should not test sympy behaviour

and also fixing 2 other conda-related failures

### 📝 Checklist

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.
    
URL: sagemath#38649
Reported by: Frédéric Chapoton
Reviewer(s): David Coudert
@vbraun vbraun merged commit aeeb93d into sagemath:develop Sep 22, 2024
20 of 21 checks passed
@fchapoton fchapoton deleted the fix_hypergeometric branch September 22, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p: CI Fix merged before running CI tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants