Opened 7 years ago

Closed 6 years ago

#17818 closed enhancement (fixed)

Interface to FindStat

Reported by: mantepse Owned by:
Priority: major Milestone: sage-6.7
Component: interfaces: optional Keywords: FindStat
Cc: vdelecroix, ncohen, stumpc5, VivianePons Merged in:
Authors: Martin Rubey Reviewers: Christian Stump, Viviane Pons
Report Upstream: N/A Work issues:
Branch: ec3bc2a (Commits, GitHub, GitLab) Commit: ec3bc2abce27759168e9d6ca5d3cb4a38a7dde4b
Dependencies: Stopgaps:

Status badges

Description (last modified by mantepse)

Sage should have an interface to http://www.findstat.org, the "combinatorial statistics finder". A first version, modelled after the interface to the OEIS is attached. To get you started, you can try the following:

sage: findstat('St000045')                   # optional -- internet
    St000045: The number of linear extensions of the tree. 

    sage: s = findstat(3); s                            # optional -- internet
    St000003: The number of [[/StandardTableaux|standard Young tableaux]] of the partition.

    sage: s.browse()

    sage: stat = {pi: pi.length() for pi in Permutations(3)}
    sage: search = findstat(stat) ; search    # optional -- internet
    0: (St000018: The [[/Permutations/Inversions|number of inversions]] of a permutation., [], 6)
    1: (St000004: The [[/Permutations/Descents-Major|major index]] of a permutation., ['inversion-number to major-index bijection'], 6)
    2: (St000067: The inversion number of the alternating sign matrix., ['to alternating sign matrix'], 6)
    3: (St000224: The sorting index of a permutation., ['first fundamental transformation'], 6)
    4: (St000008: The major index of the composition., ['inversion-number to major-index bijection', 'descent composition'], 6)
    5: (St000059: The inversion number of a standard Young tableau as defined by Haglund and Stevens. [1]  , ['inversion-number to major-index bijection', 'Robinson-Schensted recording tableau'], 6)
    6: (St000153: The number of adjacent cycles of a permutation., ['reverse', 'first fundamental transformation'], 6)
    7: (St000156: The Denert index of a permutation., ['inverse first fundamental transformation', 'foata_bijection'], 6)
    8: (St000174: The flush statistic on semistandard tableaux., ['to alternating sign matrix', 'to semistandard tableau'], 6)

Change History (121)

comment:1 Changed 7 years ago by mantepse

  • Description modified (diff)

comment:2 Changed 7 years ago by vdelecroix

  • Cc vdelecroix added

comment:3 follow-up: Changed 7 years ago by jdemeyer

I know this isn't yet set to needs_review, but the documentation is misformatted in many places. It should be like

EXAMPLES::

    sage: ...

Some text to introduce the next example::

    sage: ...

comment:4 Changed 7 years ago by ncohen

  • Cc ncohen added

comment:5 in reply to: ↑ 3 Changed 7 years ago by mantepse

You mean, the text between examples is missing?

comment:6 Changed 7 years ago by stumpc5

  • Cc stumpc5 added

comment:7 Changed 7 years ago by jdemeyer

No, I mean for example

    We can access references provided as follows:
    sage: s.references()

should be

We can access references provided as follows::

    sage: s.references()

comment:8 Changed 7 years ago by git

  • Commit changed from ce07ce4d0cb545a054f6b4878ce349119d18d901 to d21fad39a7d500d98bacc42f901942a3caa46462

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

d21fad3fix formatting for EXAMPLES and cache the right method

comment:9 follow-up: Changed 7 years ago by ncohen

Your new commit has the same problem. When you use "::" somewhere, the next line must be indented. To make sure that everything works as it should:

1) Add your new file to the documentation, i.e. the SAGE_ROOT/src/doc/en/reference/databases/index.rst file

2) compile the documentation sage -b && sage -docbuild reference/databases html

And, of course, read the html page corresponding to your new file to check that it is alright.

See also: http://www.sagemath.org/doc/developer/sage_manuals.html#adding-a-new-file

Nathann

comment:10 follow-up: Changed 7 years ago by vdelecroix

Hello,

I really do understand neither your strategy nor your goal with this ticket.

oeis is a Sage completely independent project and we have this ugly parser because we do not have any choice: there is no API to access their database.

Findstat project highly uses Sage and you are one of the author. Instead of integrating findstat code inside Sage you decided to make a closed source project. Right, this is fine with GPL and this is your choice. Now you decide to interface your findstat project with Sage. As a consequence you oblige the user to have an internet access to play with findstat features. Fine, this is again your choice. Instead of providing a clean API from the findstat side (let say json answers from http queries) you chose to implement an ugly HTML parser with regexp to get the information on the Sage side. This is inefficient and counter productive and you should seriously revise your strategy.

Vincent

comment:11 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by mantepse

I agree that the situation could be better from the sage point of view. However, there are a few things to keep in mind:

The current FindStat project really does want to be open to non-SageMath users, and it tries very hard. In fact, I believe that most of the effort went into the web interface. I think this is very important, so I make it boldface :-)

Currently, I (Martin) am mainly interested in using FindStat from within SageMath. I asked Christian whether this would be possible, and so he provided me with the necessary information to create this interface.

I agree that a clean API is desirable, but it is beyond my knowledge, and beyond the human resources of the FindStat project. (As far as I know, there are currently five people who contributed to the FindStat project -- all listed here: http://www.findstat.org/Contributors. My first and so far only contribution (apart from editing statistics) is the interface, but actually, this is more a contribution to SageMath, isn't it?)

Therefore, if you are able to provide such a clean interface, please go for it!

Last edited 7 years ago by stumpc5 (previous) (diff)

comment:12 in reply to: ↑ 11 ; follow-up: Changed 7 years ago by vdelecroix

Replying to mantepse:

I agree that the situation could be better from the sage point of view. However, there are a few things to keep in mind:

The current FindStat? project really does want to be open to non-SageMath users, and it tries very hard. In fact, I believe that most of the effort went into the web interface. I think this is very important, so I make it boldface :-)

Agreed. The website is nice as a website.

Currently, I (Martin) am mainly interested in using FindStat? from within SageMath. I asked Christian whether this would be possible, and so he provided me with the necessary information to create this interface.

This is where I do not agree. The solution to make it Sagemath-friendly is not the interface proposed in this ticket. There should be another web interface for interaction with Sage. This is not the hard part of the work.

I agree that a clean API is desirable, but it is beyond my knowledge, and beyond the human resources of the FindStat project. (As far as I know, there are currently five people who contributed to the FindStat project -- all listed here: http://www.findstat.org/Contributors. My first and so far only contribution (apart from editing statistics) is the interface, but actually, this is more a contribution to SageMath, isn't it?)

In its current version, I am not sure this interface does more good to Sagemath than to Findstat.

Therefore, if you are able to provide such a clean interface, please go for it!

I would if the code was freely available... please go release it!

Vincent

comment:13 in reply to: ↑ 12 Changed 7 years ago by mantepse

This is where I do not agree. The solution to make it Sagemath-friendly is not the interface proposed in this ticket. There should be another web interface for interaction with Sage. This is not the hard part of the work.

It would be for me. Besides, in which way precisely would this be better for SageMath?

In its current version, I am not sure this interface does more good to Sagemath than to Findstat.

I admit, this is not really important for me, I shouldn't have raised this point. I am mainly a mathematician. If this interface is useful to you, use it, otherwise don't. I was asked to polish the code so it is usable for people other than me, so I spent a few days on polishing. If it doesn't go into SageMath that's fine with me. I hope I will still be able to use it myself.

Therefore, if you are able to provide such a clean interface, please go for it!

I would if the code was freely available... please go release it!

I do not understand why you think that I am in the position to do that. FindStat? is not my website, not my code.

comment:14 in reply to: ↑ 9 ; follow-up: Changed 7 years ago by mantepse

Replying to ncohen:

Your new commit has the same problem.

2) compile the documentation sage -b && sage -docbuild reference/databases html

Hi Nathann!

Many thanks for your help, but I'm afraid I need more. Is there a way to make docbuild tell me where I made a mistake? I obtain

[databases] loading pickled environment... not yet created
[databases] building [html]: targets for 10 source files that are out of date
[databases] updating environment: 10 added, 0 changed, 0 removed
[databases] Encoding error:
[databases] 'ascii' codec can't decode byte 0xe2 in position 24871: ordinal not in range(128)
[databases] The full traceback has been saved in /tmp/sphinx-err-YMbReV.log, if you want to report the issue to the developers.
Build finished. The built documents can be found in /home/martin/sage/src/doc/output/html/en/reference/databases

but I have no idea what that traceback is trying to tell me:

Traceback (most recent call last):
  File "/home/martin/sage/local/lib/python2.7/site-packages/Sphinx-1.2.2-py2.7.egg/sphinx/cmdline.py", line 254, in main
    app.build(force_all, filenames)
  File "/home/martin/sage/local/lib/python2.7/site-packages/Sphinx-1.2.2-py2.7.egg/sphinx/application.py", line 212, in build
    self.builder.build_update()
  File "/home/martin/sage/local/lib/python2.7/site-packages/Sphinx-1.2.2-py2.7.egg/sphinx/builders/__init__.py", line 214, in build_update
    'out of date' % len(to_build))
  File "/home/martin/sage/local/lib/python2.7/site-packages/Sphinx-1.2.2-py2.7.egg/sphinx/builders/__init__.py", line 234, in build
    purple, length):
  File "/home/martin/sage/local/lib/python2.7/site-packages/Sphinx-1.2.2-py2.7.egg/sphinx/builders/__init__.py", line 134, in status_iterator
    for item in iterable:
  File "/home/martin/sage/local/lib/python2.7/site-packages/Sphinx-1.2.2-py2.7.egg/sphinx/environment.py", line 480, in update_generator
    self.read_doc(docname, app=app)
  File "/home/martin/sage/local/lib/python2.7/site-packages/Sphinx-1.2.2-py2.7.egg/sphinx/environment.py", line 627, in read_doc
    pub.publish()
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/core.py", line 217, in publish
    self.settings)
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/readers/__init__.py", line 72, in read
    self.parse()
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/readers/__init__.py", line 78, in parse
    self.parser.parse(self.input, document)
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/parsers/rst/__init__.py", line 172, in parse
    self.statemachine.run(inputlines, document, inliner=self.inliner)
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/parsers/rst/states.py", line 170, in run
    input_source=document['source'])
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/statemachine.py", line 239, in run
    context, state, transitions)
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/statemachine.py", line 460, in check_line
    return method(match, context, next_state)
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/parsers/rst/states.py", line 2726, in underline
    self.section(title, source, style, lineno - 1, messages)
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/parsers/rst/states.py", line 327, in section
    self.new_subsection(title, lineno, messages)
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/parsers/rst/states.py", line 395, in new_subsection
    node=section_node, match_titles=True)
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/parsers/rst/states.py", line 282, in nested_parse
    node=node, match_titles=match_titles)
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/parsers/rst/states.py", line 195, in run
    results = StateMachineWS.run(self, input_lines, input_offset)
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/statemachine.py", line 239, in run
    context, state, transitions)
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/statemachine.py", line 460, in check_line
    return method(match, context, next_state)
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/parsers/rst/states.py", line 2301, in explicit_markup
    self.explicit_list(blank_finish)
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/parsers/rst/states.py", line 2331, in explicit_list
    match_titles=self.state_machine.match_titles)
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/parsers/rst/states.py", line 319, in nested_list_parse
    node=node, match_titles=match_titles)
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/parsers/rst/states.py", line 195, in run
    results = StateMachineWS.run(self, input_lines, input_offset)
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/statemachine.py", line 239, in run
    context, state, transitions)
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/statemachine.py", line 460, in check_line
    return method(match, context, next_state)
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/parsers/rst/states.py", line 2604, in explicit_markup
    nodelist, blank_finish = self.explicit_construct(match)
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/parsers/rst/states.py", line 2311, in explicit_construct
    return method(self, expmatch)
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/parsers/rst/states.py", line 2054, in directive
    directive_class, match, type_name, option_presets)
  File "/home/martin/sage/local/lib/python2.7/site-packages/docutils/parsers/rst/states.py", line 2103, in run_directive
    result = directive_instance.run()
  File "/home/martin/sage/src/doc/common/sage_autodoc.py", line 1166, in run
    documenter.generate(more_content=self.content)
  File "/home/martin/sage/src/doc/common/sage_autodoc.py", line 638, in generate
    self.analyzer = ModuleAnalyzer.for_module(self.real_modname)
  File "/home/martin/sage/local/lib/python2.7/site-packages/Sphinx-1.2.2-py2.7.egg/sphinx/pycode/__init__.py", line 204, in for_module
    obj = cls.for_file(source, modname)
  File "/home/martin/sage/local/lib/python2.7/site-packages/Sphinx-1.2.2-py2.7.egg/sphinx/pycode/__init__.py", line 187, in for_file
    obj = cls(fileobj, modname, filename)
  File "/home/martin/sage/local/lib/python2.7/site-packages/Sphinx-1.2.2-py2.7.egg/sphinx/pycode/__init__.py", line 224, in __init__
    self.code = self.source.read().decode(self.encoding)
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 24871: ordinal not in range(128)

comment:15 in reply to: ↑ 14 ; follow-up: Changed 7 years ago by ncohen

Hello,

Many thanks for your help, but I'm afraid I need more. Is there a way to make docbuild tell me where I made a mistake?

[databases] Encoding error:
[databases] 'ascii' codec can't decode byte 0xe2 in position 24871: ordinal not in range(128)
[databases] The full traceback has been saved in /tmp/sphinx-err-YMbReV.log, if you want to report the issue to the developers.

Well, Sphinx says that there is a non-ascii character at position 24871 in some file. Have you checked whether there might be a non-ascii character at position 24871 in your file ?

Nathann

comment:16 in reply to: ↑ 15 ; follow-up: Changed 7 years ago by mantepse

Replying to ncohen:

Well, Sphinx says that there is a non-ascii character at position 24871 in some file. Have you checked whether there might be a non-ascii character at position 24871 in your file ?

Now I found it! It is a long dash in a reference retrieved from FindStat?. I have to redo the doctests anyway, because the descriptions, names, references, etc., are all subject to change...

Thank you!

comment:17 in reply to: ↑ 16 Changed 7 years ago by ncohen

Now I found it! It is a long dash in a reference retrieved from FindStat?. I have to redo the doctests anyway, because the descriptions, names, references, etc., are all subject to change...

There must be something wrong with dashes. Every single time I get this error message it is because of a dash between the pages in the reference of a paper, i.e. 234--190.

Let's call this message "the dash message" from now on.

Nathann

comment:18 Changed 7 years ago by VivianePons

  • Cc VivianePons added

comment:19 Changed 7 years ago by VivianePons

To answer a bit what has been said here. I do agree that it would be better to provide a proper API on the FindStat? side, that was my idea at first. But, it's not that easy, we're not that many, and nobody did it! On the other hand, Martin wanted the Sage interface because he needed it. And the dirty html parsing was the quickest way to get it. Even if it's not perfect, I still think it could be useful for other people, that's why he created the ticket.

If you want more of a technical reason: at this stage, FindStat? is no more than a customized MoinMoin? macro. Especially, we don't really deal with the http request: we just take some info from MoinMoin? and give some html back to be included in a MoinMoin? html page, we have no interface to deal with non html responses.

For similar reasons, I don't see how to include the FindStat? search directly into Sage at the moment, it demands lots of work. I'm not saying it's never going to happen, but probably not just right now.

About open-sourceness, the main reasons it's not been open yet are mostly time and technical issues. FindStat? is not a software, it's a webservice. In the meantime, we've welcomed anyone wanting to work on the project!

comment:20 Changed 7 years ago by git

  • Commit changed from d21fad39a7d500d98bacc42f901942a3caa46462 to d0ef23a9f915bc6be84f0cf4b71a2728708be2ff

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

9f16a01add findstat to documentation
d0ef23acomplete redesign, now using FindStat's ability to provide json data

comment:21 Changed 7 years ago by git

  • Commit changed from d0ef23a9f915bc6be84f0cf4b71a2728708be2ff to 6caecea9f006c0b6637025685d9ebc5b992dbc5e

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

6caeceafix oversight

comment:22 Changed 7 years ago by mantepse

I tried (with a lot of help from the FindStat? team) to answer the (technical) objections raised above. In case somebody is interested in having this finalized, I need a little help with the following items:

1) documentation and doctests

2) I believe that statistics, collections and maps should have something similar to a unique representation, but I'm not sure how to achieve this, especially for statistics. I should mention that it is quite possible that at some point collections and maps can be edited, too. So the idea is to have caching enabled for objects with id different from 0...

3) Christian told me that I need to do something like lazy import, but I currently don't really know what that is. I simply followed oeis.py...

4) a design question: it appears that, in sage, python properties are hardly used at all. I have no idea why. Currently, the interface uses properties to allow editing statistics (collections and maps currently cannot be edited). This implies some inconsistencies. Therefore:

  • should I change read-only properties like FindStatStatistic.id to ordinary methods?
  • should I do the same with the other properties, i.e., provide methods FindStatStatistic.description and FindStatStatistic.set_description?

comment:23 Changed 7 years ago by git

  • Commit changed from 6caecea9f006c0b6637025685d9ebc5b992dbc5e to 9adb2b5db3f77f1a0fcadd665bcc5cf6ee73cf77

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

9adb2b5make collections and maps hashable, fix bug for Cores, fix bug in equality for collections

comment:24 Changed 7 years ago by vdelecroix

Hello,

Looks better now ;-)

Could you use a lazy import in the "all.py". What it means is that you have to replace

from sage.databases.findstat import findstat

with

from sage.misc.lazy_import import lazy_import
lazy_import('sage.databases.findstat', 'findstat')

What it changes is that you will have:

sage: type(findstat)
<type 'sage.misc.lazy_import.LazyImport'>

but mostly nothing else (access to method, tab completion, documentation is identical). The advantage is that it is not imported on startup. It would be really good since you are doing a lot of global imports in your module.

Vincent

comment:25 Changed 7 years ago by git

  • Commit changed from 9adb2b5db3f77f1a0fcadd665bcc5cf6ee73cf77 to 8817ab3025c69927894e5928a5c9f7aa5062e879

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

8817ab3lazy import findstat

comment:26 Changed 7 years ago by git

  • Commit changed from 8817ab3025c69927894e5928a5c9f7aa5062e879 to 05a08edd8a0f0a8532a1fba6fead85e4d97e42de

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

05a08edmore doctesting, fix bug in FindStatCollection.__init__

comment:27 Changed 7 years ago by vdelecroix

Plenty of documentation issues! Did you try to run make doc?

  1. After INPUT:, OUTPUT: there must be a blankline.
  1. TODO sections must be indented, i.e.
    .. TODO::
    
        we definitely should do this and that
    
  1. The first line of the documentation should be an imperative and start with an uppercase. Replace
    returns the name of the collection
    
    with
    Return the name of the collection
    
    And no point at the end of this first sentence.
  1. This is wrong and will raise an error during the creation of the documentation.
    EXAMPLES::
    
    A particular statistic can be called by its St-number or number::
    
    There should be two colon only if the next block is indented. In that case it should be
    EXAMPLES:
    
    A particular statistic can be called by its St-number or number::
    
    And in the following you forgot a colon
    To search for a distribution, send a list of lists, or a single pair:
    

There is a lot of useful information in the developer guide.

comment:28 Changed 7 years ago by git

  • Commit changed from 05a08edd8a0f0a8532a1fba6fead85e4d97e42de to fefc6c2a8e466e5afaa608be6f3ffe540b21e26f

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

fefc6c2improving documentation: make doc passes

comment:29 Changed 7 years ago by git

  • Commit changed from fefc6c2a8e466e5afaa608be6f3ffe540b21e26f to 14def60afb6300516bda1b4b0d1d8c277a154f4c

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

14def60better formatting and more documentation

comment:30 Changed 7 years ago by git

  • Commit changed from 14def60afb6300516bda1b4b0d1d8c277a154f4c to 52330b9e17e7c58aa335900af13cfb4ccaccea83

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

52330b9unicode tempfile

comment:31 Changed 7 years ago by git

  • Commit changed from 52330b9e17e7c58aa335900af13cfb4ccaccea83 to 61d0c69546ac4ca4c8151ab319aabff17e94bc42

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

7c407bdMerge git://trac.sagemath.org/sage into public/findstat-interface
fd6b6c3Merge branch 'public/findstat-interface' of git://trac.sagemath.org/sage into public/findstat-interface
4c48615Merge branch 'public/findstat-interface' of git://trac.sagemath.org/sage into public/findstat-interface
61d0c69properly html-escape

comment:32 Changed 6 years ago by git

  • Commit changed from 61d0c69546ac4ca4c8151ab319aabff17e94bc42 to 0702e72f4f2626dd5fee2e8db6d1311f2215ea6e

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

0702e72replace properties by setters, improve documentation

comment:33 Changed 6 years ago by git

  • Commit changed from 0702e72f4f2626dd5fee2e8db6d1311f2215ea6e to 4ba6ef012b4581b8dc5bd277c556c8290b84d441

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

4ba6ef0more documentation, bugfix in in_range, findstat accepts FindStatCollection

comment:34 Changed 6 years ago by mantepse

  • Status changed from new to needs_review

comment:35 Changed 6 years ago by stumpc5

  • Status changed from needs_review to needs_work

Hi Martin, thanks for all your work on that!

  1. Several doctests currently fail:
sage -t src/sage/databases/findstat.py
**********************************************************************
File "src/sage/databases/findstat.py", line 637, in sage.databases.findstat.FindStatStatistic.id
Failed example:
    findstat(1).id()                              # optional -- internet
Expected:
    <BLANKLINE>
    (process:...): GLib-CRITICAL **: g_slice_set_config: assertion 'sys_page_size == 0' failed
    1
Got:
    1
**********************************************************************
File "src/sage/databases/findstat.py", line 719, in sage.databases.findstat.FindStatStatistic.function
Failed example:
    findstat(lambda pi: pi.length(), "Permutations").function() # optional -- internet
Expected:
    <function <lambda> at ...>
Got:
    <BLANKLINE>
    (process:21380): GLib-CRITICAL **: g_slice_set_config: assertion 'sys_page_size == 0' failed
    <function <lambda> at 0x7fb420e806e0>
**********************************************************************
File "src/sage/databases/findstat.py", line 1294, in sage.databases.findstat.FindStatCollection.first_terms
Failed example:
    c.first_terms(lambda x: 1, max_values=10))                       # optional -- internet
Exception raised:
    Traceback (most recent call last):
      File "/home/stumpc5/progs/sage-git/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 488, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/stumpc5/progs/sage-git/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 848, in compile_and_execute
        compiled = compiler(example)
      File "/home/stumpc5/progs/sage-git/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 486, in <lambda>
        example.source, filename, "single", compileflags, 1)
      File "<doctest sage.databases.findstat.FindStatCollection.first_terms[2]>", line 1
        c.first_terms(lambda x: Integer(1), max_values=Integer(10)))                       # optional -- internet
                                                                   ^
    SyntaxError: invalid syntax
**********************************************************************
File "src/sage/databases/findstat.py", line 1505, in sage.databases.findstat.FindStatMap.id
Failed example:
    m = findstat(lambda pi: pi.length(), "Permutations")[1][1][0] # optional -- internet
Expected nothing
Got:
    <BLANKLINE>
    (process:21392): GLib-CRITICAL **: g_slice_set_config: assertion 'sys_page_size == 0' failed
    <BLANKLINE>
    (process:21402): GLib-CRITICAL **: g_slice_set_config: assertion 'sys_page_size == 0' failed
**********************************************************************
4 items had failures:
   1 of   4 in sage.databases.findstat.FindStatCollection.first_terms
   1 of   3 in sage.databases.findstat.FindStatMap.id
   1 of   2 in sage.databases.findstat.FindStatStatistic.function
   1 of   2 in sage.databases.findstat.FindStatStatistic.id
    [77 tests, 4 failures, 12.80 s]
----------------------------------------------------------------------
sage -t src/sage/databases/findstat.py  # 4 doctests failed
----------------------------------------------------------------------
Total time for all tests: 13.1 seconds
    cpu time: 5.7 seconds
    cumulative wall time: 12.8 seconds
  1. The documentation doesn't build (unless I made a mistake, I haven't been building docs in a while):
Error building the documentation.

Note: incremental documentation builds sometimes cause spurious
error messages. To be certain that these are real errors, run
"make doc-clean" first and try again.
Traceback (most recent call last):
  File "/home/stumpc5/progs/sage-git/src/doc/common/builder.py", line 1618, in <module>
    getattr(get_builder(name), type)()
  File "/home/stumpc5/progs/sage-git/src/doc/common/builder.py", line 503, in _wrapper
    x.get(99999)
  File "/home/stumpc5/progs/sage-git/local/lib/python/multiprocessing/pool.py", line 558, in get
    raise self._value
OSError: [databases] /home/stumpc5/progs/sage-git/local/lib/python2.7/site-packages/sage/databases/findstat.py:docstring of sage.databases.findstat:16: ERROR: Unexpected indentation.
  1. I wonder whether it is okay that doctests open browser windows, @vdelecroix, @ncohen.
  1. How should doctests that change from the FindStat? side over time be handled, @vdelecroix, @ncohen. One (possibly not the best) example is
    sage: [FindStatCollection(c.first_terms(lambda x: 0, max_values=1)[0][0]) for c in cl]
        [Cc0001: Permutations,
         Cc0002: Integer partitions,
         Cc0005: Dyck paths,
         Cc0006: Integer compositions,
         Cc0007: Standard tableaux,
         Cc0009: Set partitions,
         Cc0010: Binary trees,
         Cc0012: Perfect matchings,
         Cc0013: Cores,
         Cc0014: Posets,
         Cc0017: Alternating sign matrices,
         Cc0018: Gelfand-Tsetlin patterns,
         Cc0019: Semistandard tableaux,
         Cc0020: Graphs,
         Cc0021: Ordered trees,
         Cc0022: Finite Cartan types,
         Cc0023: Parking functions]

comment:36 Changed 6 years ago by stumpc5

I was trying to fix the doctests, but stumble across the following problem: I constantly get errors such as

Failed example:
    FindStatCollection("Binary trees").name()               # optional -- internet
Expected:
    u'Binary tree'
Got:
    <BLANKLINE>
    (process:23292): GLib-CRITICAL **: g_slice_set_config: assertion 'sys_page_size == 0' failed
    u'Binary tree'

and the problem is that this

    <BLANKLINE>
    (process:23292): GLib-CRITICAL **: g_slice_set_config: assertion 'sys_page_size == 0' failed

only happens sometimes (google search tells me this appears to be a firefox issue, but I wouldn't know how to handle this in Sage doctests.

comment:37 Changed 6 years ago by git

  • Commit changed from 4ba6ef012b4581b8dc5bd277c556c8290b84d441 to 598ac1e8ceaacc000cc38bf1818e1636a5b63d15

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

598ac1e- Removed the colons of the end of the first doc lines.

comment:38 Changed 6 years ago by git

  • Commit changed from 598ac1e8ceaacc000cc38bf1818e1636a5b63d15 to 5b39d53191356f852771b256fd1d609c2cc0429e

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

5b39d53fixed typo in the documentation that make it fail to build

comment:39 Changed 6 years ago by mantepse

Christian, the last commit is not following the conventions at http://www.sagemath.org/doc/developer/coding_basics.html#the-docstring-of-a-function-content:

It [the one sentence description] must be followed by a blank line and end in a period.

Please revert.


New commits:

5b39d53fixed typo in the documentation that make it fail to build

comment:40 Changed 6 years ago by mantepse

I'm still having the problem that I cannot run the doctests at all. As I mentioned on sage-devel, I get, for example:

martin@Martin-Laptop:~/sage$ ./sage -t --optional=internet src/sage/databases/findstat.py
too few successful tests, not using stored timings
Running doctests with ID 2015-03-30-10-41-21-fba377bd.
Git branch: public/findstat-interface
Doctesting 1 file.
sage -t src/sage/databases/findstat.py
**********************************************************************
File "src/sage/databases/findstat.py", line 1152, in sage.databases.findstat.FindStatCollection.__init__
Failed example:
    FindStatCollection("Dyck paths")                 # optional -- internet
Exception raised:
    Traceback (most recent call last):
      File "/home/martin/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 488, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/martin/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 850, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.databases.findstat.FindStatCollection.__init__[0]>", line 1, in <module>
        FindStatCollection("Dyck paths")                 # optional -- internet
    NameError: name 'FindStatCollection' is not defined

How can I fix this? FindStatCollection? is imported in the preceding line of the docstring!

comment:41 Changed 6 years ago by stumpc5

Is I wrote some time earlier, you must run

sage -t --optional=sage,internet path/to/interface.sage

If you only run --optional=internet, you are *only* running those tests having # optional -- internet, so all the imports (and a few others) are not performed.

comment:42 Changed 6 years ago by mantepse

Even for a ".py" file? OK, yes, it works. Thanks!

Regarding your comments above:

3.) I agree. Removing these doctests also gets rid of the annoying firefox bug.

4.) This test is there precisely because *if* it doesn't pass, the interface *should* be updated (in the case at hand, with the information concerning new combinatorial collections.

Could you please revert your commit with the periods, then I fix the other stuff! Thanks also for fixing the docbuild!

comment:43 follow-up: Changed 6 years ago by stumpc5

4.) This test is there precisely because *if* it doesn't pass, the interface *should* be updated (in the case at hand, with the information concerning new combinatorial collections.

My point here is: doctests can stop working without any change on the Sage side. This implies that a new Sage release possibly depends on someone fixing the doctests in the FindStat interface. I am not sure this is the right thing to do...

comment:44 Changed 6 years ago by stumpc5

(the doctests were working, but I now get back the unexpected indention, that's why I haven't yet pushed.

comment:45 Changed 6 years ago by git

  • Commit changed from 5b39d53191356f852771b256fd1d609c2cc0429e to d604a77e06fda46c4f560ee267a033eda1672855

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

d604a77- Added back the periods at the end of the first line of the docs.

comment:46 in reply to: ↑ 43 ; follow-up: Changed 6 years ago by mantepse

Replying to stumpc5:

4.) This test is there precisely because *if* it doesn't pass, the interface *should* be updated (in the case at hand, with the information concerning new combinatorial collections.

My point here is: doctests can stop working without any change on the Sage side. This implies that a new Sage release possibly depends on someone fixing the doctests in the FindStat interface. I am not sure this is the right thing to do...

Well, but if this doctest (as currently written) doesn't pass, the interface will not work reliably: user might get a result where her statistic is produced via a map from a statistic on the new collection.

The constant _findstat_collections is really the only place that needs updating, and it should be easy to update it. But it does need updating...


New commits:

d604a77- Added back the periods at the end of the first line of the docs.

comment:47 Changed 6 years ago by stumpc5

Okay, found the problem. I added your examples from the class FindStat? also to the beginning of the file in order to make the reference manual nicer to read. Feel free to update that again if you prefer another way of doing it.

comment:48 in reply to: ↑ 46 Changed 6 years ago by stumpc5

Replying to mantepse:

Well, but if this doctest (as currently written) doesn't pass, the interface will not work reliably: user might get a result where her statistic is produced via a map from a statistic on the new collection.

I am not necessarily arguing about doctest that tests whether the interface still works well (even though my general concern also applies to those, and e.g. the OEIS interface was broken several times IIRC).

What about the doctest

sage: print findstat(1).description() # optional -- internet
The number of ways to write a permutation as a minimal length product of simple transpositions.

after someone has changed the description of this statistic? Or the case that we added another collection -- would that break the interface, or would it only not serve the new collection as long as it is not added?

comment:49 Changed 6 years ago by mantepse

Yes, I agree that there is a problem with the other doctests, involving name, description, code, perhaps even values... But I don't really know what to do about this. I think it would be a shame if the interface cannot go in just because of this.

A workaround might be to mark these EXAMPLES as random. Once full search is implemented, I could have doctests that search for the description. But I won't do this now.

The only doctest which is really necessary is the test in the FindStatCollection? class.

comment:50 Changed 6 years ago by mantepse

Concerning the introductory section: I'm going to think of a systematic tour, which demonstrates one or two typical workflows. Thanks for catching the error!

Concerning the webbrowser: the simple and correct solution is to remove the keyword "internet" from the webbrowser tests!

comment:51 Changed 6 years ago by git

  • Commit changed from d604a77e06fda46c4f560ee267a033eda1672855 to b97b209f902f1ba0bcf89d459b89d43c605ed0b9

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

e7ed192separate internet and webbrowser, fix a doctest
b97b209Merge branch 'public/findstat-interface' of git://trac.sagemath.org/sage into public/findstat-interface

comment:52 Changed 6 years ago by git

  • Commit changed from b97b209f902f1ba0bcf89d459b89d43c605ed0b9 to 6752f09313131d7fe4fc0b91cb4dbd10a3e8dece

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

6752f09make doctest random whenever necessary

comment:53 Changed 6 years ago by git

  • Commit changed from 6752f09313131d7fe4fc0b91cb4dbd10a3e8dece to 8272b8337c1cdf230fbb53c291684b2a79aebae8

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

8272b83cosmetics for doctests

comment:54 Changed 6 years ago by git

  • Commit changed from 8272b8337c1cdf230fbb53c291684b2a79aebae8 to 51ff4ef005cbca824a9b471ce310b4669f2f9e02

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

51ff4efmake code of maps available, add todo for introduction

comment:55 Changed 6 years ago by git

  • Commit changed from 51ff4ef005cbca824a9b471ce310b4669f2f9e02 to cd07f7769907d815498b30471804a55d637f8cfc

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

cd07f77unified the FINDSTAT_URL parameters

comment:56 Changed 6 years ago by git

  • Commit changed from cd07f7769907d815498b30471804a55d637f8cfc to 01e7ca429c1064cef0c0ffca643b9826456c8f8f

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

01e7ca4include guided tour, expose name of code of maps, fixes to references, equality and inequality

comment:57 Changed 6 years ago by mantepse

  • Status changed from needs_work to needs_review

comment:58 Changed 6 years ago by git

  • Commit changed from 01e7ca429c1064cef0c0ffca643b9826456c8f8f to 5212c0184e61ebd162ca856d8f61f8195f2405b8

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

5212c01fix missing parenthesis. I hate python.

comment:59 Changed 6 years ago by git

  • Commit changed from 5212c0184e61ebd162ca856d8f61f8195f2405b8 to 6159c1ca2b75e3234e0042bfb9f1770299ab6200

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

6159c1cbetter unicode support

comment:60 Changed 6 years ago by chapoton

  • Status changed from needs_review to needs_work

Two things (see patchbot report)

  • please use python3-style raise statements:
    raise Error("this is the good way")
    

instead of

raise Error, 'this was the old way'

The bot says

Old-style raise statement inserted on 5 non-empty lines
  • please make sure that every function is doctested.

'Missing doctests ', 'databases/findstat.py', '40 / 57 = 70%')

comment:61 Changed 6 years ago by git

  • Commit changed from 6159c1ca2b75e3234e0042bfb9f1770299ab6200 to 72b6e380856116d6757fa5179b5f698606223701

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

72b6e38replace old-style raise statements

comment:62 follow-up: Changed 6 years ago by mantepse

Thank you for raising my awareness!

Two questions:

1) where can I find the patchbot report?

2) how am I supposed to doctest functions that use a webbrowser?

comment:63 Changed 6 years ago by chapoton

1) Patchbot reports are available by clicking on the round colored icon (top right of the page)

2) I do not know, sorry. I have not looked at your code.

comment:64 Changed 6 years ago by mantepse

1) great! Thank you!

2) So, I guess I have to ask on sage-devel...

comment:65 in reply to: ↑ 62 Changed 6 years ago by vdelecroix

Replying to mantepse:

2) how am I supposed to doctest functions that use a webbrowser?

Just have a look at the oeis module. Though I would not say that doctesting all functions is a requirement.

comment:66 Changed 6 years ago by mantepse

Thanks Frederic and Vincent! I just looked at

sage --coverage findstat.py

which more or less tells me what to do. I guess I'll simply cook up some tests this afternoon.

comment:67 Changed 6 years ago by git

  • Commit changed from 72b6e380856116d6757fa5179b5f698606223701 to 0abffb86d9bd89c44d5903ba0534c0f3df3cf698

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

0abffb8documentation and doctest fixes

comment:68 Changed 6 years ago by git

  • Commit changed from 0abffb86d9bd89c44d5903ba0534c0f3df3cf698 to 2ed6389647ca2b9752f38d733f1922323cdb4a09

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

2ed6389more docfixes

comment:69 Changed 6 years ago by git

  • Commit changed from 2ed6389647ca2b9752f38d733f1922323cdb4a09 to 1d93a015afb7d92a59559b12f132b212298ca40f

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

1d93a01yet more doctests

comment:70 Changed 6 years ago by git

  • Commit changed from 1d93a015afb7d92a59559b12f132b212298ca40f to 238cd2c8ce6cfa77ffda487ac81c4c9b4dad1c95

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

238cd2cfix problem with determining whether an object is used in a FindStat search

comment:71 Changed 6 years ago by mantepse

  • Status changed from needs_work to needs_review

comment:72 Changed 6 years ago by mantepse

I cannot think of any more sensible tests...

comment:73 Changed 6 years ago by chapoton

  • Milestone changed from sage-6.6 to sage-6.7

comment:74 Changed 6 years ago by git

  • Commit changed from 238cd2c8ce6cfa77ffda487ac81c4c9b4dad1c95 to f89c50a79f9687f9608940ba6fa5ecd7f95ab6be

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

f89c50aeven more docfixes

comment:75 Changed 6 years ago by git

  • Commit changed from f89c50a79f9687f9608940ba6fa5ecd7f95ab6be to f8393b67bb1a3592b392835715df647893cf3b0c

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

f8393b6example demonstrating a common pitfall

comment:76 Changed 6 years ago by git

  • Commit changed from f8393b67bb1a3592b392835715df647893cf3b0c to ef1def71af41d9efb0fad4e265e850fe3a936ceb

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

ef1def7docfix

comment:77 Changed 6 years ago by chapoton

Please avoid using # in the doctests, as for example here:

# when one of these tests does not pass, there is probably a new collection to be added.

comment:78 Changed 6 years ago by git

  • Commit changed from ef1def71af41d9efb0fad4e265e850fe3a936ceb to 7456e56c019b1c0f6637ef3d75d53caf990fda4a

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

e90491afix test for FindStatCollection, add example to submit
7456e56Merge branch 'public/findstat-interface' of git://trac.sagemath.org/sage into public/findstat-interface

comment:79 Changed 6 years ago by git

  • Commit changed from 7456e56c019b1c0f6637ef3d75d53caf990fda4a to 9252e3b9fe5b36f522e3fa2069107dd4bd032caa

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

9252e3bdon't even try to modify query results with perfect match

comment:80 Changed 6 years ago by git

  • Commit changed from 9252e3b9fe5b36f522e3fa2069107dd4bd032caa to d025f46445e6ededdbe640eea05772ade8349c61

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

d025f46try to make patchbot happier

comment:81 Changed 6 years ago by stumpc5

I just did

sage: A = findstat({ S: len([B for B in S if len(B) == 1 ] ) for S in SetPartitions(4) },depth=0)
sage: A.submit()
ValueError                                Traceback (most recent call last)
...
ValueError: too many values to unpack

Instead, this worked:

sage: def statistic(S):
    return len([B for B in S if len(B) == 1 ])

sage: A = findstat(statistic,SetPartitions,depth=0)
sage: A.submit()

It took me a few minutes to figure how to properly give a statistic in order to submit it -- wouldn't it be nice if the first version would also work, or at least give a hint on how to do it?

comment:82 Changed 6 years ago by mantepse

Are you sure you have the most recent version? I get:

sage: A = findstat({ S: len([B for B in S if len(B) == 1 ] ) for S in SetPartitions(4) },depth=0)
sage: A
0: (St000247: The number of singleton blocks of a set partition., [], 15)
sage: A.submit()
ValueError: Your input data matches St000247.  Consider modifying this statistic instead.

comment:83 Changed 6 years ago by stumpc5

haha -- I used the second version, submitted the statistic, viviane reviewed it, and you now find it from the interface...

comment:84 Changed 6 years ago by mantepse

Oh. I'm about to fix this! Thanks for the bug report!

comment:85 Changed 6 years ago by git

  • Commit changed from d025f46445e6ededdbe640eea05772ade8349c61 to 4c5a8dcff3fe42d73f67a21e1a2c3a7c0871038e

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

210d624convert first_terms to list of tuples when supplying a dict
4c5a8dcMerge branch 'public/findstat-interface' of git://trac.sagemath.org/sage into public/findstat-interface

comment:86 Changed 6 years ago by git

  • Commit changed from 4c5a8dcff3fe42d73f67a21e1a2c3a7c0871038e to 01f52d5e01420acb3445f2be050b5fb1f8253a48

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

01f52d5fix typo

comment:87 Changed 6 years ago by mantepse

Done! Thanks again!

comment:88 Changed 6 years ago by git

  • Commit changed from 01f52d5e01420acb3445f2be050b5fb1f8253a48 to b633bd3558e456e0d756f4af9d8040c431ad231f

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

b633bd3Merge branch 'develop' into public/findstat-interface

comment:89 Changed 6 years ago by git

  • Commit changed from b633bd3558e456e0d756f4af9d8040c431ad231f to 800182391f9e34662f8f8b8fbfde275eb5390468

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

a09ea67Small fixes
8001823Small fixes: adding an extra condition on the __eq__ methods to avoid

comment:90 Changed 6 years ago by VivianePons

  • Reviewers set to Christian Stump, Viviane Pons

I've read through the whole ticket and documentation: everything seems fine. I've fixed the few little things I've noticed. It seems like everything that had been requested has been done. The design is very reasonable and the tests pass!

So, if everyone's happy I would suggest that Christian or Martin review my latest push and that we put a positive review.

Thank you Martin, I find it a very nice work.

comment:91 Changed 6 years ago by stumpc5

Great, seems good to me, but I let Martin to have the last check on your changes.

comment:92 Changed 6 years ago by mantepse

Wow, great!

Because of lack of a computer I cannot really test your changes, so please forgive if I could answer the following questions myself:

  • does it really make sense to make the class FindStat inherit from SageObject? I thought the rule was for classes with "mathematical" objects, or at least objects the user can actually manipulate or inspect. The reason I did not use it was that the class FindStat actually should not produce any objects...
  • I think I recall that in __eq__
        if (not isinstance(other, FindStatStatistic)):
             return False
    
    is actually unnecessary - but I'm not sure. Could you please double-check? (same for FindStatMap)

In any case: yes, I'm very happy with having this finished!

Thank you so much!

Martin

comment:93 Changed 6 years ago by VivianePons

You're right about SageObject? it might not be necessary. I was under the impression that the FindStat? class did return an object but maybe the user can never see this object...

About the eq I did check and it seemed necessary: when I was comparing statisctics or whatever to other things (like integers) I would get an ugly Exception instead of just False. Maybe there is another way to do this. I was actually under the impression that it should have been taken care for by SageObject? but it doesn't seem so...

comment:94 Changed 6 years ago by mantepse

Great!

comment:95 Changed 6 years ago by mantepse

Hi Viviane!

I have just created two more classes FindStatCollections and FindStatMaps which serve as parents for collections and maps. Although nice, this is not a very important feature, I think. So I don't want to delay the review because of that.

However, if you are willing to review this in the same ticket I would push it to trac! (Note that I'm a bit unsure whether I did things correctly with all this classcall magic.)

comment:96 Changed 6 years ago by git

  • Commit changed from 800182391f9e34662f8f8b8fbfde275eb5390468 to dd50895ccfdcdde5b86595d84db990fe06a42bac

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

6dced39introduce parents for maps and collections
dd50895implement comparison for FindStatMaps and FindStatCollections properly

comment:97 Changed 6 years ago by mantepse

Sorry Viviane, it's so much better that I pushed. I hope you can give it your OK :-)

comment:98 Changed 6 years ago by git

  • Commit changed from dd50895ccfdcdde5b86595d84db990fe06a42bac to dc6ff91e7a96966a2485cdf61c30cce9dfc513ac

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

dc6ff91provide pickling for maps and collections

comment:99 Changed 6 years ago by VivianePons

No problem. I ave still one week of sage days so I will have plenty of time to review. And you're right, it's probably better with proper parents.

comment:100 Changed 6 years ago by jdemeyer

  • Component changed from interfaces to interfaces: optional

comment:101 Changed 6 years ago by stumpc5

Hi Viviane --

I wanted to check what the status of the review here is... also: what is the difference between interfaces and interfaces: optional

Christian

comment:102 Changed 6 years ago by git

  • Commit changed from dc6ff91e7a96966a2485cdf61c30cce9dfc513ac to 8777b30330c5cf5f02a4a6c69669ff089a76ee63

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

8777b30correct and improve error handling

comment:103 Changed 6 years ago by mantepse

I just noticed that the interface is slightly inconsistent: it expects either objects and values, or a statistic in form of a callable and a collection. I guess the collection should come first.

comment:104 Changed 6 years ago by git

  • Commit changed from 8777b30330c5cf5f02a4a6c69669ff089a76ee63 to ffc90a85672c745f6fad2a20b6e18b20002e598b

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

ffc90a8change order of arguments when providing a statistic in form of a callable

comment:105 Changed 6 years ago by chapoton

  • Status changed from needs_review to needs_work

does not build, see patchbot report (metaclass problem)

comment:106 Changed 6 years ago by git

  • Commit changed from ffc90a85672c745f6fad2a20b6e18b20002e598b to e727d044735ce75ff917dc92e7532dd20d48346d

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

11ba3ceMerge branch 'master' of git://trac.sagemath.org/sage into public/findstat-interface
e727d04fix metaclass failure, improve documentation, increase doctest coverage

comment:107 Changed 6 years ago by mantepse

I have no idea why, but it seems that I needed to replace

__metaclass__ = ClasscallMetaclass

with

__metaclass__ = InheritComparisonClasscallMetaclass

Just took me a few hours :-(

comment:108 Changed 6 years ago by mantepse

  • Status changed from needs_work to needs_review

comment:109 Changed 6 years ago by stumpc5

The patchbot says that there are two missing doctests. Please recheck!

comment:110 Changed 6 years ago by git

  • Commit changed from e727d044735ce75ff917dc92e7532dd20d48346d to 482caecc0ac94237a8b8053b6418ecb6fd8eedce

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

482caecprovide doctests to make the patchbot happy, improve documentation

comment:111 Changed 6 years ago by git

  • Commit changed from 482caecc0ac94237a8b8053b6418ecb6fd8eedce to 21c4812b58991aff37914c2948d7409151ea1ed7

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

21c4812Merge branch 'develop' into public/findstat-interface

comment:112 Changed 6 years ago by git

  • Commit changed from 21c4812b58991aff37914c2948d7409151ea1ed7 to f841404abb56f2a7629e373b198e1482c84a5285

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

f841404moved a few things in the documentation

comment:113 Changed 6 years ago by stumpc5

I have been playing with the interface for some time now, and I consider it to be ready to get merged. Martin did a great job and the "guided tour" is an awesome point of entry to use the interface, with several easy examples to follow! When they pop up in real life, it might be worth to add also some more sophisticated use cases.

All tests pass on my machine with the optional flags set, I still wait for the patchbot to be green again (though it does not do any doctests since they all require an internet connection) to give it a positive review.

comment:114 Changed 6 years ago by stumpc5

  • Status changed from needs_review to positive_review

comment:115 Changed 6 years ago by vbraun

  • Status changed from positive_review to needs_work

Attempts internet access:

sage -t --long src/sage/databases/findstat.py
**********************************************************************
File "src/sage/databases/findstat.py", line 844, in sage.databases.findstat.FindStatStatistic._find_by_id
Failed example:
    findstat(999999)                                              # indirect doctest
Expected:
    Traceback (most recent call last):
    ...
    ValueError: St999999 is not a FindStat statistic identifier.
Got:
    <BLANKLINE>
    Traceback (most recent call last):
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 496, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 858, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.databases.findstat.FindStatStatistic._find_by_id[0]>", line 1, in <module>
        findstat(Integer(999999))                                              # indirect doctest
      File "sage/misc/lazy_import.pyx", line 383, in sage.misc.lazy_import.LazyImport.__call__ (/Users/buildslave-sage/slave/sage_git/build/src/build/cythonized/sage/misc/lazy_import.c:3593)
        return self._get_object()(*args, **kwds)
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/databases/findstat.py", line 476, in __call__
        return self._statistic_find_by_id_cached(query)
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/databases/findstat.py", line 631, in _statistic_find_by_id_cached
        self._statistic_cache[id] = FindStatStatistic(id)._find_by_id()
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/databases/findstat.py", line 855, in _find_by_id
        self._raw = json.load(urlopen(url), object_pairs_hook=OrderedDict)
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python/urllib2.py", line 154, in urlopen
        return opener.open(url, data, timeout)
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python/urllib2.py", line 431, in open
        response = self._open(req, data)
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python/urllib2.py", line 449, in _open
        '_open', req)
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python/urllib2.py", line 409, in _call_chain
        result = func(*args)
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python/urllib2.py", line 1227, in http_open
        return self.do_open(httplib.HTTPConnection, req)
      File "/Users/buildslave-sage/slave/sage_git/build/local/lib/python/urllib2.py", line 1197, in do_open
        raise URLError(err)
    URLError: <urlopen error [Errno 60] Operation timed out>
**********************************************************************
1 item had failures:
   1 of   2 in sage.databases.findstat.FindStatStatistic._find_by_id
    [50 tests, 1 failure, 75.18 s]

comment:116 Changed 6 years ago by stumpc5

  • Status changed from needs_work to needs_review

Isn't that part of the nature of an internet access that a connection can time out? That also happened to me before, but when rerunning the tests, everything went well. How should doctests that need an internet connection be handled otherwise?

comment:117 Changed 6 years ago by git

  • Commit changed from f841404abb56f2a7629e373b198e1482c84a5285 to ec3bc2abce27759168e9d6ca5d3cb4a38a7dde4b

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

ec3bc2aadded the optional flag to one test

comment:118 Changed 6 years ago by stumpc5

Sorry, I think the problem was that this test was missing the flag optional -- internet, that's why you got the doctest failure. Was that is?


New commits:

ec3bc2aadded the optional flag to one test

comment:119 Changed 6 years ago by stumpc5

Martin, would you briefly look at my change, do the doctests, and set the ticket back to positive review. Thanks!

comment:120 Changed 6 years ago by mantepse

  • Status changed from needs_review to positive_review

Well, the change is trivial - I only didn't know that I'm entitled to "positively review" it...

(Besides, I do not understand why you moved the definitions of collection, map and statistic, but I admit that I do not really care... In the same commit, there is also a typo: "entriy" should by "entry". Apart from that, I agree that the changes are an improvement! Thanks!)

comment:121 Changed 6 years ago by vbraun

  • Branch changed from public/findstat-interface to ec3bc2abce27759168e9d6ca5d3cb4a38a7dde4b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.