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

17 internet doctest failing in findstat.py #28864

Closed
seblabbe opened this issue Dec 10, 2019 · 117 comments
Closed

17 internet doctest failing in findstat.py #28864

seblabbe opened this issue Dec 10, 2019 · 117 comments

Comments

@seblabbe
Copy link
Contributor

findstat.org has now a proper api. In this ticket we switch to this api, and create a hierarchy of classes that better models what findstat provides.

In particular, this allows search for maps, iterating over all statistics or maps with given domain, easier access to search for distributions, etc.

Restricted to the capabilities of the old interface, the usage remains the same.

We also fix all the failing doctest.

Original ticket description

Using SageMath version 9.2.beta2, Release Date: 2020-06-26, the command

sage -t --optional=sage,internet src/sage/databases/findstat.py

gives

**********************************************************************
7 items had failures:
   4 of  16 in sage.databases.findstat
   3 of  10 in sage.databases.findstat.FindStat
   1 of  11 in sage.databases.findstat.FindStat.__call__
   1 of  10 in sage.databases.findstat.FindStatCollection.in_range
   3 of   5 in sage.databases.findstat.FindStatStatistic.first_terms
   3 of   5 in sage.databases.findstat.FindStatStatistic.generating_functions
   2 of   4 in sage.databases.findstat.FindStatStatistic.oeis_search
    [249 tests, 17 failures, 41.24 s]
----------------------------------------------------------------------
sage -t src/sage/databases/findstat.py  # 17 doctests failed
----------------------------------------------------------------------

CC: @mantepse @stumpc5

Component: combinatorics

Keywords: FindStat

Author: Martin Rubey

Branch/Commit: 52c576e

Reviewer: Sébastien Labbé

Issue created by migration from https://trac.sagemath.org/ticket/28864

@seblabbe seblabbe added this to the sage-9.0 milestone Dec 10, 2019
@fchapoton
Copy link
Contributor

comment:1

any idea of what the problem could be ? some problem for empty shape, maybe ?

@mantepse
Copy link
Collaborator

comment:2

Essentially the problem is (very very likely) that there is a new collection on findstat.org (skew partitions), which is not reflected in the sage interface (yet).

Very unfortunately, findstat.org itself has (technical) problems which I was unable to resolve so far.

On top of that, investing time into the old code is going to delay the deployment of the new and shiny and proper api for the website even further, although it is almost done (since an embarassingly long time).

One way to speed up the process would be to decide to drop support for the old way to access findstat completely. This would mean that, that I would switch findstat.org and the sage interface to the new api simultaneously, and that old versions of sage could not use findstat anymore. I doubt that this is a very good idea, but it is a possibility.

@mantepse
Copy link
Collaborator

mantepse commented Jan 2, 2020

comment:3

a brief update:

  • I am now finished with implementing a proper api on the server side of FindStat, it is currently in testing - findstat.org itself is using this api since 31.12.2019.

  • My next task is to rewrite the sage-findstat interface, I will use this as a testcase to check whether the api is well designed. I intend to publish the api and the new interface at the same time.

In the meantime, the sage interface should be fully functional.

@embray
Copy link
Contributor

embray commented Jan 6, 2020

comment:4

Ticket retargeted after milestone closed

@embray embray modified the milestones: sage-9.0, sage-9.1 Jan 6, 2020
@mantepse
Copy link
Collaborator

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 13, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

eb29c9ebegin fixing doctests, add some more infrastructure

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 13, 2020

Commit: eb29c9e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 13, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

d1b2f29make all statistics by domain accessible via findstat

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 13, 2020

Changed commit from eb29c9e to d1b2f29

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 14, 2020

Changed commit from d1b2f29 to 696e79d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 14, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

696e79dmake cache for statistics global

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 14, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

5070c5ffix iterating over maps and statistics, fix doctests

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 14, 2020

Changed commit from 696e79d to 5070c5f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 14, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

64b685cfix and improve argument parsing in findmap

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 14, 2020

Changed commit from 5070c5f to 64b685c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 15, 2020

Changed commit from 64b685c to b5ad6e9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 15, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

b5ad6e9fix 3rd positional argument for findmap, fix doctests for offline

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 15, 2020

Changed commit from b5ad6e9 to 243d0b8

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 15, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

243d0b8make tab completion work

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 16, 2020

Changed commit from 243d0b8 to 725e6a3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 16, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

725e6a3doc fixes

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 16, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

e7c68c4py3 fixes

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 16, 2020

Changed commit from 725e6a3 to e7c68c4

@stumpc5
Copy link
Contributor

stumpc5 commented Jan 17, 2020

comment:15

Here are some first comments:

  • """
    There is a new collection available at The Combinatorial Statistic Finder (https://www.findstat.org/): Signed permutations.
    To use it with this interface, it has to be added to the dictionary
    SupportedFindStatCollections in src/sage/databases/findstat.py of the SageMath distribution.
    Please open a ticket on trac!
    """

    Would it be possible to automatize this? Otherwise, every user will see this on startup and likely get confused.

  • I would prefer a single entry point (maybe FindStat with FindStat.find_statistic and FindStat.find_map?). If I know findstat but not findmap, I might completely miss the latter? Also, using FindStat? would immediately give instructions how to proceed.

  • (This is not for this ticket but for the backend:) Queries from the interface are not yet logged in the findstat log.

  • 41: St001489oMp00081oMp00059 (quality [100, 100]) The term quality is not obviously explained. In the doc, it might be referred to as "a number which says how many values...". a proper definition of this quality might be helpful.

  • The term distribution is not explained how to be used in the input section of findstat: it is twice referred to as "distribution must be None".

  • It remains unclear (I think, to a new user) what "a new statistic" means. I would suggest to not show this as one of the "results" of a DababaseQuery. Instead, I would suggest to have this available as a method submit_as_new_statistic.

    • This method fails if the input datum is not a statistic (but a distribution).
    • If there is no result, the user gets prompted a message that this method is available in case she wants to submit the data.
  • The term offset is not exlained, is it?

  • What about two methods matching values and unmatched_values ?

    • Each of these could consist of lists of pairs of the form ( ( elt1, elt2, elt3 ), val ).
  • Finally: Since the output might be long and confusing, I think it would be great if FindStatMatchingStatistic had a method info or detailed_info or alike which would in detail present this match:

    sage: A = findstat("Permutations", distribution = lambda pi: pi.number_of_descents())
    sage: a = A[37]; a
    St001169oMp00012oMp00061 (quality [100, 100])
    sage: a.info()
    After applying
    
      Mp00061: to increasing tree
      Mp00012: to Dyck path: up step, left tree, down step, right tree
      (see `a.maps()` for details)
    
    your input data matches
    
      St001169: Number of simple modules with projective dimension at least two in the corresponding Nakayama algebra.
      (see `a.statistic()` for details)
    
    with offset 0 in xxx / yyy many values and xxx/yyy many different values (see `a.matching_values()` and `a.unmatched_values()` for details).
    

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 13, 2020

Changed commit from dec07aa to bdefdf2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 13, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

bdefdf2fix doctest of hash

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 13, 2020

Changed commit from bdefdf2 to 085b693

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 13, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

085b693fix some pyflakes warnings

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 14, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

4a8cb1ffix another pyflakes warning

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 14, 2020

Changed commit from 085b693 to 4a8cb1f

@seblabbe
Copy link
Contributor Author

comment:63

python3 says:

inside file:  b/src/sage/databases/findstat.py
@@ -171,13 +205,12 @@ Classes and methods
+from six import string_types
Python3 incompatible code inserted on 1 non-empty lines

Is there a more python3 way to import it? Maybe among:

sage: import_statements('string_types')                                         
# **Warning**: distinct objects with name 'string_types' in:
#   - ipython_genutils.py3compat
#   - pexpect.utils
#   - pickleshare
#   - pygments.filters
#   - pygments.formatter
#   - pygments.formatters.html
#   - pygments.util
#   - six
#   - traitlets.utils.importstring
# ** Warning **: several modules for the object (<class 'str'>,): pygments.filters, pygments.formatter, pygments.formatters.html, pygments.util
from pygments.util import string_types

Also pyflakes still says:

src/sage/combinat/decorated_permutation.py:23:1 'sage.misc.classcall_metaclass.ClasscallMetaclass' imported but unused

found 1 pyflakes errors in the modified files

Is it really unused?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 14, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

e236ef0refactor to use ClonableArray

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 14, 2020

Changed commit from 4a8cb1f to e236ef0

@mantepse
Copy link
Collaborator

comment:66

Hi Sébastien!

Thank you for looking into this!

Do you happen to know the replacement for string_types? (The interface was incredibly sensitive to str-unicode issues in the past, which is why I'm a bit hesitant.)

@fchapoton
Copy link
Contributor

comment:67

now that sage is python3 only, we should not use six at all. Use str

And use the python3 syntax for metaclass. git grep will help you to find existing examples.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 14, 2020

Changed commit from e236ef0 to 52c576e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 14, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

52c576eremove string_types

@mantepse
Copy link
Collaborator

comment:69

Concerning metaclass: isn't

class FindStatMap(Element,
                  FindStatFunction,
                  FindStatCombinatorialMap,
                  metaclass=InheritComparisonClasscallMetaclass):

correct?

I replaced string_types with str


New commits:

52c576eremove string_types

@mantepse
Copy link
Collaborator

comment:71

(I guess I should also update the ticket description, right?)

@mantepse

This comment has been minimized.

@seblabbe
Copy link
Contributor Author

comment:73

python3 + pyflakes now ok. Doc builds ok (according to patchbot). All normal test passed on patchbot (except a unrelated singular error). All tests including internet pass on my machine for the file findstat. Positive review.

@mantepse
Copy link
Collaborator

comment:74

Wow, that was quick! Thank you!

@vbraun
Copy link
Member

vbraun commented Sep 18, 2020

Changed branch from u/mantepse/1_internet_doctest_failing_in_findstat_py to 52c576e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants