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

systematically avoid checking of input #35233

Conversation

mantepse
Copy link
Collaborator

@mantepse mantepse commented Mar 5, 2023

Trusting that the code is correct, there is no reason to check the input.

📚 Description

Closes #35212, #35212 (comment)

📝 Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • I have linked an issue or discussion.

⌛ Dependencies

@mantepse
Copy link
Collaborator Author

mantepse commented Mar 5, 2023

@tmiller, that's part two.

@mantepse
Copy link
Collaborator Author

mantepse commented Mar 5, 2023

Two questions:

  1. should we rename check_input to check. check_input is only used in three methods:
combinat/skew_tableau.py:    def to_ribbon(self, check_input=True):
graphs/generic_graph.py:    def relabel(self, perm=None, inplace=True, return_map=False, check_input=True, complete_partial_function=True, immutable=None):
matrix/matrix2.pyx:    def jordan_form(self, base_ring=None, sparse=False, subdivide=True, transformation=False, eigenvalues=None, check_input=True):
  1. I'd like to catch future performance regressions. In particular:
sage: P = Permutations(10)
sage: %%time
....: for pi in P:
....:     pass
CPU times: user 14.5 s, sys: 62 µs, total: 14.5 s
Wall time: 14.5 s

now gives

CPU times: user 8.43 s, sys: 0 ns, total: 8.43 s
Wall time: 8.43 s

@tscrim
Copy link
Collaborator

tscrim commented Mar 5, 2023

  1. I think check is better than check_input (and more standard). +1 on changing it (albeit with a slightly annoying deprecation).
  2. We can't really doctest our way to catching performance regressions (at least on that scale). We need a separate timings test package/files. I don't think this is worth the time/effort to do anything about it here.

@codecov-commenter
Copy link

codecov-commenter commented Mar 5, 2023

Codecov Report

Patch coverage: 98.23% and project coverage change: +0.02 🎉

Comparison is base (52a81cb) 88.57% compared to head (c9be738) 88.59%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #35233      +/-   ##
===========================================
+ Coverage    88.57%   88.59%   +0.02%     
===========================================
  Files         2140     2140              
  Lines       397273   397419     +146     
===========================================
+ Hits        351891   352111     +220     
+ Misses       45382    45308      -74     
Impacted Files Coverage Δ
src/sage/schemes/elliptic_curves/ell_generic.py 93.25% <66.66%> (+0.01%) ⬆️
src/sage/interfaces/tachyon.py 87.93% <90.00%> (+0.43%) ⬆️
src/sage/schemes/elliptic_curves/gal_reps.py 82.23% <90.00%> (+0.04%) ⬆️
src/sage/quadratic_forms/quadratic_form.py 90.26% <95.65%> (+0.16%) ⬆️
src/sage/combinat/dyck_word.py 96.78% <100.00%> (ø)
src/sage/combinat/permutation.py 96.65% <100.00%> (+<0.01%) ⬆️
src/sage/modular/quasimodform/element.py 99.20% <100.00%> (+0.06%) ⬆️
src/sage/rings/qqbar.py 95.30% <100.00%> (+<0.01%) ⬆️
src/sage/schemes/affine/affine_morphism.py 90.33% <100.00%> (ø)
src/sage/schemes/elliptic_curves/BSD.py 43.75% <100.00%> (+0.21%) ⬆️
... and 71 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mantepse
Copy link
Collaborator Author

mantepse commented Mar 5, 2023

  1. I think check is better than check_input (and more standard). +1 on changing it (albeit with a slightly annoying deprecation).

will do!

2. We can't really doctest our way to catching performance regressions (at least on that scale). We need a separate timings test package/files. I don't think this is worth the time/effort to do anything about it here.

I know, I was referring to #35046.

@mantepse
Copy link
Collaborator Author

mantepse commented Mar 5, 2023

There are very many test failures, but I cannot reproduce them locally :-(

@github-actions
Copy link

github-actions bot commented Mar 5, 2023

Documentation preview for this PR is ready! 🎉
Built with commit: c9be738

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Thank you. LGTM. The tester now seems to be passing without any problems too.

@mantepse
Copy link
Collaborator Author

Thank you! I'm afraid you have to remove the needs-review label and add the positive-review label manually, right?

@tscrim
Copy link
Collaborator

tscrim commented Mar 10, 2023

Sorry, I forgot to do this. Done.

@vbraun
Copy link
Member

vbraun commented Mar 26, 2023

merge conflict

@vbraun vbraun merged commit f5815de into sagemath:develop Apr 1, 2023
@mkoeppe mkoeppe added this to the sage-10.0 milestone Apr 1, 2023
@mantepse mantepse deleted the do_not_unnecessarily_check_input_in_iterators branch April 1, 2023 17:40
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.

Permutation(n) doesn't use the cache if there is one.
5 participants