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

Adding a new collection to FindStat breaks the interface #19543

Closed
stumpc5 opened this issue Nov 7, 2015 · 16 comments
Closed

Adding a new collection to FindStat breaks the interface #19543

stumpc5 opened this issue Nov 7, 2015 · 16 comments

Comments

@stumpc5
Copy link
Contributor

stumpc5 commented Nov 7, 2015

The interface is now broken due to the newly added collection. There should be a way to avoid this break.

CC: @mantepse

Component: interfaces

Keywords: FindStat

Author: Martin Rubey

Branch/Commit: u/mantepse/develop @ 151934e

Reviewer: Christian Stump

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

@stumpc5 stumpc5 added this to the sage-6.10 milestone Nov 7, 2015
@mantepse
Copy link
Contributor

mantepse commented Nov 7, 2015

Branch: u/mantepse/develop

@mantepse
Copy link
Contributor

mantepse commented Nov 7, 2015

Commit: 3c17593

@stumpc5
Copy link
Contributor Author

stumpc5 commented Nov 8, 2015

comment:3

Looks good to me -- do you also want to use this ticket to add the support of binary words, or do you want to create another?

@mantepse
Copy link
Contributor

mantepse commented Nov 8, 2015

comment:4

I thought of putting the binary words in another ticket, because otherwise this one cannot be tested...

@stumpc5
Copy link
Contributor Author

stumpc5 commented Nov 8, 2015

comment:5

I am not sure whether that's the right way to go -- e.g., currently all doctests of findstat.py fail due to this print.

@stumpc5
Copy link
Contributor Author

stumpc5 commented Nov 8, 2015

Reviewer: Christian Stump

@stumpc5
Copy link
Contributor Author

stumpc5 commented Nov 8, 2015

Author: Martin Rubey

@stumpc5
Copy link
Contributor Author

stumpc5 commented Nov 8, 2015

comment:7

Here is another problem to fix:

sage: findstat({l:l.length() for l in Partitions(4)})

---------------------------------------------------------------------------
IOError                                   Traceback (most recent call last)
...
IOError: FindStat did not answer with a json response: Could not find FindStat collection for Binary words.

Before, we had the problem that FindStat was broken, so the thrown error was

IOError: FindStat did not answer with a json response: St000288 is not a FindStat statistic identifier.

I think that the finder functionality should never throw an error if one of the results is corrupted in any way (this way, the interface is robust even if FindStat is broken).

One quick idea: what about having a method findstat.collections such that

sage: findstat.collections()
{'Binary words': 'not supported',
 'Dyck paths': 'supported',
 'Permutations': 'supported'}

and if an unsupported collection is used, we throw an error like the one you have.
Good:

sage: findstat(289)
ValueError: Could not find FindStat collection for Binary words.

but unclear what to do in the above situation where a binary word statistic is found and we cannot throw an error...

@mantepse
Copy link
Contributor

mantepse commented Nov 9, 2015

comment:8

Oh no! Thanks for pointing this out! Almost a fix is committed.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 9, 2015

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

151934ealmost a fix

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 9, 2015

Changed commit from 3c17593 to 151934e

@mantepse
Copy link
Contributor

comment:10

This is subsumed by #19296.

@mantepse mantepse removed this from the sage-6.10 milestone Nov 10, 2015
@fchapoton
Copy link
Contributor

comment:12

can we close that now ?

@stumpc5
Copy link
Contributor Author

stumpc5 commented Nov 15, 2015

comment:14

I believe so. @mantepse ?

@mantepse
Copy link
Contributor

comment:15

Yes, but I don't know how to do this. I set it to wontfix, is this not enough?

@fchapoton
Copy link
Contributor

comment:16

you need to set to positive_review, then the release manager will close

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

4 participants