Opened 7 years ago
Closed 7 years ago
#19296 closed enhancement (fixed)
Generating function in FindStat interface
Reported by: | stumpc5 | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.10 |
Component: | packages: optional | Keywords: | FindStat, OEIS |
Cc: | mantepse | Merged in: | |
Authors: | Christian Stump, Martin Rubey | Reviewers: | Martin Rubey, Christian Stump |
Report Upstream: | N/A | Work issues: | document |
Branch: | 15ec356 (Commits, GitHub, GitLab) | Commit: | 15ec3561001bbb9f6372e4d245959f6a2adb470a |
Dependencies: | Stopgaps: |
Description (last modified by )
Change History (115)
comment:1 Changed 7 years ago by
- Branch set to u/stumpc5/generating_function_in_findstat_interface
comment:2 Changed 7 years ago by
- Cc mantepse added
- Commit set to eefeaae4deb2df18fb97cc8458090139b9eb2c31
- Description modified (diff)
- Keywords FindStat OEIS added
- Type changed from PLEASE CHANGE to enhancement
- Work issues set to document
comment:3 Changed 7 years ago by
- Commit changed from eefeaae4deb2df18fb97cc8458090139b9eb2c31 to 0a69285394e85831ae4de65caa5e10267621001d
Branch pushed to git repo; I updated commit sha1. New commits:
0a69285 | cosmetic
|
comment:4 Changed 7 years ago by
@mantepse, do I need to add the utf8 encoding in
self._generating_function = self._raw[FINDSTAT_STATISTIC_GENERATING_FUNCTION]
as in some of the other similar lines before?
comment:5 Changed 7 years ago by
- Commit changed from 0a69285394e85831ae4de65caa5e10267621001d to b0eda2842a96ea3438d255a369da48f4c83066c9
Branch pushed to git repo; I updated commit sha1. New commits:
b0eda28 | remved a leftover print command
|
comment:6 Changed 7 years ago by
- Commit changed from b0eda2842a96ea3438d255a369da48f4c83066c9 to 6640df6c8d5e2fbfc4ced4fb8f1ba4545e23fa69
Branch pushed to git repo; I updated commit sha1. New commits:
6640df6 | added name_plural method
|
comment:7 Changed 7 years ago by
- Commit changed from 6640df6c8d5e2fbfc4ced4fb8f1ba4545e23fa69 to e1b0c89914a9b2cc75d16a5d04f7b68bbc942f61
Branch pushed to git repo; I updated commit sha1. New commits:
e1b0c89 | added verbose and improved the oeis search
|
comment:8 Changed 7 years ago by
- Commit changed from e1b0c89914a9b2cc75d16a5d04f7b68bbc942f61 to 9926f7f0431ccac0fc6b5fe4947422c3984ff65c
Branch pushed to git repo; I updated commit sha1. New commits:
9926f7f | added max length to oeis search
|
comment:9 Changed 7 years ago by
- Commit changed from 9926f7f0431ccac0fc6b5fe4947422c3984ff65c to 83d5332fac33521569e7882e4ee17aded984d559
Branch pushed to git repo; I updated commit sha1. New commits:
83d5332 | improved max length to oeis search
|
comment:10 Changed 7 years ago by
- Commit changed from 83d5332fac33521569e7882e4ee17aded984d559 to f544c28f4b0c0c4707c017f60d45a62b58501c76
comment:11 follow-up: ↓ 13 Changed 7 years ago by
I think it would be better to have the commits added name to json and pull it from there
and added name_plural method
in a separate ticket. Actually, I even have a bug to fix...
comment:12 follow-up: ↓ 16 Changed 7 years ago by
@mantepse If I now push the new collections.json
without the CollectionLevelsPrecomputed
field. Is then the current version of the interface braking? I guess so, right?
comment:13 in reply to: ↑ 11 ; follow-up: ↓ 14 Changed 7 years ago by
Replying to mantepse:
I think it would be better to have the commits
added name to json and pull it from there
andadded name_plural method
in a separate ticket. Actually, I even have a bug to fix...
We can do that if you want...
comment:14 in reply to: ↑ 13 Changed 7 years ago by
comment:15 Changed 7 years ago by
- Commit changed from f544c28f4b0c0c4707c017f60d45a62b58501c76 to cde7b5bdce31462f4046451bbff30026ece0a920
Branch pushed to git repo; I updated commit sha1. New commits:
44da776 | cleaned the statistic name method
|
836f205 | improved statistic name, added collection name plural, some cosmetic changes as well
|
b95a704 | Merge branch 't/19307/improve_statistic_name_and_add_collection_name_plural_to_findstat_interface' into t/19296/generating_function_in_findstat_interface
|
cde7b5b | removed everything that is now in #19307
|
comment:16 in reply to: ↑ 12 Changed 7 years ago by
Replying to stumpc5:
@mantepse If I now push the new
collections.json
without theCollectionLevelsPrecomputed
field
Yes, if you do this then there is a breakage, although trivial to fix. I'm afraid you can get rid of CollectionLevelsPrecomputed
only after the trivial fix is merged, which will very likely take some time.
That's great that you thought of this!
comment:17 Changed 7 years ago by
- Commit changed from cde7b5bdce31462f4046451bbff30026ece0a920 to 8f093a0014bad991b957738f97207abdccd877de
comment:18 Changed 7 years ago by
- Commit changed from 8f093a0014bad991b957738f97207abdccd877de to ce7214ac8a76d5a2cd6339eff21a2b5be126d186
Branch pushed to git repo; I updated commit sha1. New commits:
ce7214a | added two lines that were missing from the diverging
|
comment:19 Changed 7 years ago by
- Commit changed from ce7214ac8a76d5a2cd6339eff21a2b5be126d186 to 8df3e6390fb74de9a8d300b40032a464cd139992
comment:20 Changed 7 years ago by
- Component changed from PLEASE CHANGE to packages: optional
- Status changed from new to needs_review
comment:21 Changed 7 years ago by
- Branch changed from u/stumpc5/generating_function_in_findstat_interface to u/mantepse/generating_function_in_findstat_interface
comment:22 Changed 7 years ago by
I fixed two trivial doctesting problems, and I have a few questions and suggestions:
1) the docstring of oeis_search
Returns the OEIS search for the generating function of ``self``.
could be improved: what's returned is actually the result of an OEIS search. So perhaps:
Search the OEIS for the generating function of the statistic.
2) also, there should be an OUTPUT
section, perhaps
OUTPUT: - a tuple of OEIS sequences, see :meth:`OEIS.find_by_description` for more information.
3) not sure, but I think the blank-line print
are unconventional.
4) the EXAMPLES
section does not display correctly in html.
comment:23 Changed 7 years ago by
- Commit changed from 8df3e6390fb74de9a8d300b40032a464cd139992 to 54051ea405a43a33531668ba002a38414c0098df
Branch pushed to git repo; I updated commit sha1. New commits:
54051ea | fix two docstring problems
|
comment:24 Changed 7 years ago by
- Branch changed from u/mantepse/generating_function_in_findstat_interface to u/stumpc5/generating_function_in_findstat_interface
comment:25 Changed 7 years ago by
- Commit changed from 54051ea405a43a33531668ba002a38414c0098df to 304ceca132e718ea355eb279966091f230b3532f
Branch pushed to git repo; I updated commit sha1. New commits:
304ceca | took care of Martin's suggestions (all -- I didn't check the html output)
|
comment:26 Changed 7 years ago by
- Commit changed from 304ceca132e718ea355eb279966091f230b3532f to 8582fb12c8f3f7d0a03b794018745abc08167b99
Branch pushed to git repo; I updated commit sha1. New commits:
8582fb1 | forgot to resolve a merge conflict
|
comment:27 Changed 7 years ago by
- Commit changed from 8582fb12c8f3f7d0a03b794018745abc08167b99 to 5558be983f10469e5be31e45652364437a0edb03
Branch pushed to git repo; I updated commit sha1. New commits:
5558be9 | I still had some problems with the merging, hope that's fixed now
|
comment:28 Changed 7 years ago by
- Dependencies set to #19307
New commits:
5558be9 | I still had some problems with the merging, hope that's fixed now
|
comment:29 follow-up: ↓ 31 Changed 7 years ago by
The html now looks almost fine, with the exception of the link to OEIS.find_by_description()
, which does not work.
There is another, less minor thing: generating_functions
currently only works for queries that yield statistics, i.e., of the form findstat(number)
:
sage: s=findstat("Permutations", number_of_alignments); s 0: (St000222: The number of alignments of a permutation, [], 200) sage: s.generating_functions() ... AttributeError: 'FindStatStatistic' object has no attribute '_generating_function' sage: s[0][0].generating_functions() {2: 2, 3: 3*q + 3, 4: 12*q^2 + 8*q + 4, 5: 20*q^4 + 45*q^3 + 35*q^2 + 15*q + 5, 6: 100*q^6 + 180*q^5 + 198*q^4 + 140*q^3 + 72*q^2 + 24*q + 6}
While this is as expected, I think it shouldn't be that way. In particular, if possible it would be nice to be able to use oeis_search
even for new statistics.
Of course, this requires that the generating function is computed by the sage interface, and not retrieved from findstat.org.
I'll try to propose a patch shortly.
comment:30 Changed 7 years ago by
- Status changed from needs_review to needs_work
The generating function as list does not work for negative statistic values.
comment:31 in reply to: ↑ 29 Changed 7 years ago by
Replying to mantepse:
The html now looks almost fine, with the exception of the link to
OEIS.find_by_description()
, which does not work.
would you mind fixing this? I have built the documentation at the moment for a different ticket, so that would take me forever.
There is another, less minor thing:
generating_functions
currently only works for queries that yield statistics, i.e., of the formfindstat(number)
:
Good catch! Looking forward to see your solution!
See also the other issue I just posted... I will provide a fix for that!
comment:32 Changed 7 years ago by
Here is another question concerning negative statistic values: so far, I had considered the generating function 2q + q^2
as the sequence 0,2,1
where the initial 0
corresponds to the constant term 0
. But if I consider negative statistic values, I might end up with the generating function q^{-1} + 2q
, which I have to move to the sequence 1,0,2
.
So, should I always shift the sequence of a generating function such that it starts with the first nonzero term?
[I find this counter intuitive for nonnegative statistics, but it is unavoidable for partially negative statistics.]
Finally, it seems special wired if my generating functions are
f_1(q) = q^{-1} + q f_2(q) = 2q + q^2
and then shift the first, but not the second, to finally get 1,0,1,0,2,1
.
comment:33 Changed 7 years ago by
I don't think I understand. Where do you convert sequences to generating functions (or the other way round)?
comment:34 Changed 7 years ago by
The generating function is well-defined for any integer valued statistic (though it may be a Laurent polynomial). If I want to search the OEIS for the generating function of a statistic, I have to turn the generating function into a sequence, or indeed a linearized table T(n,k) where T(n,k) is the coefficient of q^k
in the generating function of the n-th piece of the statistic (say the permutations of n or the Dyck paths of semilength n,...).
But since k \geq 0, this does not work for Laurent polynomials.
comment:35 Changed 7 years ago by
OK, so that's really a question on how to fine-tune oeis searches. Don't you separate the individual generating functions with blanks? If so, I would start with the first non-zero coefficient.
comment:36 Changed 7 years ago by
OK, so that's really a question on how to fine-tune oeis searches. Don't you separate the individual generating functions with blanks? If so, I would start with the first non-zero coefficient.
Indeed I agree, and that's what I do already. But I would also like to use this functionality to provide a linear sequence and submit it to the OEIS. And for that I do not separate the individual generating functions.
comment:37 Changed 7 years ago by
But since this is not really an issue for the Sage interface to FindStat, I should maybe not consider it here...
comment:38 follow-up: ↓ 40 Changed 7 years ago by
I discovered an interesting user-interface problem: when doing a search for distribution FindStat? does not expect that the objects within one block of values are all on the same level. Eg, the following is a valid findstat query:
[1,2] [2,1] [1,2,3] [1,3,2] [2,1,3] [2,3,1] [3,1,2] [3,2,1] ====> 1,1,2,2,2,2,2,2
But of course, in this case there is no way to compute the generating function...
comment:39 Changed 7 years ago by
- Branch changed from u/stumpc5/generating_function_in_findstat_interface to u/mantepse/generating_function_in_findstat_interface
comment:40 in reply to: ↑ 38 Changed 7 years ago by
- Commit changed from 5558be983f10469e5be31e45652364437a0edb03 to 2d85a2ec9a47908f6f707c714281f57595e093ff
Replying to mantepse:
I discovered an interesting user-interface problem: when doing a search for distribution FindStat? does not expect that the objects within one block of values are all on the same level.
What is the problem with that? I implemented this as a feature!
New commits:
54051ea | fix two docstring problems
|
1652db2 | Merge branch 'u/stumpc5/generating_function_in_findstat_interface' of git://trac.sagemath.org/sage into u/mantepse/generating_function_in_findstat_interface
|
2d85a2e | draft of generating function from data
|
comment:41 Changed 7 years ago by
Are you planning to remove all dependencies on CollectionLevelsPrecomputed
in this ticket? I thought to have that in an indiviual ticket, but it's also fine this way...
comment:42 Changed 7 years ago by
- Commit changed from 2d85a2ec9a47908f6f707c714281f57595e093ff to 167d39b44d87fc41d901dcfbe97a6cacf9f5522d
Branch pushed to git repo; I updated commit sha1. New commits:
167d39b | integrate new generating functions code
|
comment:43 Changed 7 years ago by
Could you please have a look? The current solution uses the http://FindStat.org delivered data for findstat(number)
, and computes the gf's otherwise.
comment:44 Changed 7 years ago by
- Branch changed from u/mantepse/generating_function_in_findstat_interface to u/stumpc5/generating_function_in_findstat_interface
comment:45 Changed 7 years ago by
- Commit changed from 167d39b44d87fc41d901dcfbe97a6cacf9f5522d to ccd121c9f00b669f4106358549dd00a0ceecc6dd
Branch pushed to git repo; I updated commit sha1. New commits:
ccd121c | fixed a little bug in the generating function code
|
comment:46 Changed 7 years ago by
- Commit changed from ccd121c9f00b669f4106358549dd00a0ceecc6dd to 78e9a6087a56e8b1bfb808b49b9e0bdec4d5092e
Branch pushed to git repo; I updated commit sha1. New commits:
78e9a60 | shoot, typo
|
comment:47 Changed 7 years ago by
sage: Pi = Permutations(3).list()*2 sage: st = findstat([Pi,[pi.major_index() for pi in Pi]]) hi --------------------------------------------------------------------------- ValueError Traceback (most recent call last) ... ValueError: FindStat delivers more statistic values than the level has elements. This should not happen. Please send and email to the developers.
Hm, that's too bad, isn't it?
comment:48 Changed 7 years ago by
Unsurprisingly, the same happens for
sage: st = findstat([(pi,pi.major_index()) for pi in Pi])
and even for
sage: st = findstat([(pi,pi.major_index() ) for pi in Permutations(3)] + [(pi,pi.major_index()+1) for pi in Permutations(3)] )
The later is indeed not a statistic at all!
comment:49 Changed 7 years ago by
excellent, that's a bug in the preparation of _data
comment:50 Changed 7 years ago by
- Commit changed from 78e9a6087a56e8b1bfb808b49b9e0bdec4d5092e to 4adcd928d977788bd4a50ccbc648e0ae89c9b15e
Branch pushed to git repo; I updated commit sha1. New commits:
4adcd92 | fixed two bugs (one of mine and one of Martin) in the generating function code
|
comment:51 Changed 7 years ago by
Why did you add if gfs[key]
to the loop for key in sorted(gfs.keys())
? That seems to produce wrong outputs...
comment:52 Changed 7 years ago by
Because I made compute_generating_functions
use None
if there are values for the level, but not enough. I guess that's stupid, so feel free to remove it.
comment:53 Changed 7 years ago by
- Branch changed from u/stumpc5/generating_function_in_findstat_interface to u/mantepse/generating_function_in_findstat_interface
comment:54 Changed 7 years ago by
- Commit changed from 4adcd928d977788bd4a50ccbc648e0ae89c9b15e to 36d52b37ea2f71b6e3f186f83b7033fa57ba56ad
what is the point of having the lines
if len(set(elements)) != len(elements): raise ValueError("FindStat expects that every object appears only once!")
twice? Making the first if len(set(elements)) != len(elements)
into an if len(set(all_elements)) != len(all_elements)
should make the second obsolete. Or am I overseeing sth. or do you not like that?
New commits:
36d52b3 | check that user does not provide an object twice
|
comment:55 Changed 7 years ago by
Well, you can change it if you like. I didn't like to turn all_elements
into a set on every iteration, that's all. So, currently in every iteration we check that the elements in one block are distinct, and then we check that there is no element appearing in two different blocks.
We could actually simply delete the first check, that's probably clearest.
comment:56 Changed 7 years ago by
- Branch changed from u/mantepse/generating_function_in_findstat_interface to u/stumpc5/generating_function_in_findstat_interface
comment:57 Changed 7 years ago by
- Branch changed from u/stumpc5/generating_function_in_findstat_interface to u/mantepse/generating_function_in_findstat_interface
We could actually simply delete the first check, that's probably clearest.
agreed and pushed.
comment:58 Changed 7 years ago by
- Branch changed from u/mantepse/generating_function_in_findstat_interface to u/stumpc5/generating_function_in_findstat_interface
comment:59 Changed 7 years ago by
- Commit changed from 36d52b37ea2f71b6e3f186f83b7033fa57ba56ad to 1798c37a7f0e962f5e72bb60c47722e8fbc249e4
I still don't know how to properly interact with the branches, but it seems that there is nothing better than constantly merging and changing the branch on trac...
New commits:
b23dac2 | modified the list generating function so that it can handle negative statistic values
|
347c026 | merged Martin any my changes
|
ccd121c | fixed a little bug in the generating function code
|
78e9a60 | shoot, typo
|
4adcd92 | fixed two bugs (one of mine and one of Martin) in the generating function code
|
bf2c4b6 | Merge remote-tracking branch 'trac/u/mantepse/generating_function_in_findstat_interface' into t/19296/generating_function_in_findstat_interface
|
1798c37 | cleaned two lines of code as discussed
|
comment:60 Changed 7 years ago by
- Branch changed from u/stumpc5/generating_function_in_findstat_interface to u/mantepse/generating_function_in_findstat_interface
comment:61 Changed 7 years ago by
- Branch changed from u/mantepse/generating_function_in_findstat_interface to u/stumpc5/generating_function_in_findstat_interface
I have now removed name_plural
and the change to _name
, because they have a separate ticket and induced a doctest failure.
Do we need to open another ticket for the bugfix barking on duplicate objects? (I do not care) Otherwise, I think this is ready to go. probably we need to doctest compute_generating_functions
to make the patchbot happy.
comment:62 Changed 7 years ago by
- Branch changed from u/stumpc5/generating_function_in_findstat_interface to u/mantepse/generating_function_in_findstat_interface
comment:63 Changed 7 years ago by
- Commit changed from 1798c37a7f0e962f5e72bb60c47722e8fbc249e4 to 5b42946bef35abc01c15c56f09ae19ee2030686c
Okay, if two tickets A and B do two different things, but to develop what I want in B I need A. How am I supposed to be doing that without merging A into B and having A as a dependency in B?
New commits:
5b42946 | remove name_plural and change to name, because these belong to ticket #19307
|
comment:64 Changed 7 years ago by
- Dependencies #19307 deleted
comment:65 Changed 7 years ago by
- Commit changed from 5b42946bef35abc01c15c56f09ae19ee2030686c to 1fe70f7eaa757c9c2cf1bdde415bce9e22d3fb15
Branch pushed to git repo; I updated commit sha1. New commits:
1fe70f7 | do not produce None in _compute_generating_functions, add doctest, fix hyperlinks
|
comment:66 Changed 7 years ago by
- Reviewers set to Martin Rubey
- Status changed from needs_work to positive_review
New commits:
1fe70f7 | do not produce None in _compute_generating_functions, add doctest, fix hyperlinks
|
comment:67 Changed 7 years ago by
- Status changed from positive_review to needs_info
Sorry to get back to this, but if len(set(all_elements)) != len(all_elements)
assumes the elements to be hashable.
FindStat? internally this is totally fine, but that makes the following, very natural, findstat search fail:
sage: findstat( { g:g.wiener_index() for g in Graphs(4) if g.is_connected() } ) --------------------------------------------------------------------------- ... TypeError: This graph is mutable, and thus not hashable. Create an immutable copy by `g.copy(immutable=True)`
It will obviously take much longer to do the uniqueness test in a list, but I guess it's worth it. What do you think?
comment:68 Changed 7 years ago by
The line you sent won't even work if you correct the typo (Graphs
-> graphs
) and omit findstat
.
But otherwise you are right. Possibly we should normalize the keys at the beginning, maybe even store the string representations.
comment:69 Changed 7 years ago by
I don't know how I got that line wrong, it should read
findstat([ (g,g.wiener_index()) for g in graphs(4) if g.is_connected() ])
and now only the findstat hashing causes the problem (and it was working without this ticket).
Possibly we should normalize the keys at the beginning, maybe even store the string representations.
+1. Are you going to work on this and the addition of the set_first_terms
, or should I do one/both of these?
comment:70 Changed 7 years ago by
- Commit changed from 1fe70f7eaa757c9c2cf1bdde415bce9e22d3fb15 to 4d6e9eb840d59a6b2b3b0446b1162d19eb6dc893
Branch pushed to git repo; I updated commit sha1. New commits:
4d6e9eb | bugfix
|
comment:71 Changed 7 years ago by
Wow, that's a whole lot of code for a bug I would have fixed by changing one line (but I am not saying my solution would have been the better)!
I wonder:
Derzeit werden alle normalisiert, auch wenn's 17 Millionen sind
- Are really more normalized than used? A search should be restricted to
FINDSTAT_MAX_???
and a new statistic toFINDSTAT_MAX_***
. Do you now normalize also all the others?
- is it really better (implementation-wise and time-wise) to store also all string representations than to check whether some list is duplicate-free? Or are you / do you plan using that string representation also somewhere else?
comment:72 Changed 7 years ago by
Wow, that's a whole lot of code for a bug I would have fixed by changing one line (but I am not saying my solution would have been the better)!
I'd be interested in seeing your solution.
I wonder:
Derzeit werden alle normalisiert, auch wenn's 17 Millionen sind
Are really more normalized than used? A search should be restricted to
FINDSTAT_MAX_???
and a new statistic toFINDSTAT_MAX_***
. Do you now normalize also all the others?
I'm afraid so. However, if the normalization really takes too long, I would move the code in _find_by_values
that restricts the amount of data to FINDSTAT_MAX_???
to FindStat.__call__
and only normalize those data which are sent to FindStat?. If there is no problem, I certainly do not want to do this.
is it really better (implementation-wise and time-wise) to store also all string representations than to check whether some list is duplicate-free? Or are you / do you plan using that string representation also somewhere else?
How could I check without normalization that the list contains no duplicates?
comment:73 follow-up: ↓ 75 Changed 7 years ago by
Just a quick issue along the way:
sage: S = Permutations() sage: a,b,c = S([1,2,3]),S([1,3,2]),S([2,1,3]) sage: findstat( [([a],[1]),([b],[1])] ) a new statistic on Cc0001: Permutations
Shouldn't that return an error as too little values are given?
comment:74 Changed 7 years ago by
Okay, the following is really nice as you do it:
sage: g = Graph([[1,2],[2,3]]) sage: h = Graph([[1,3],[2,3]]) sage: i = Graph([[1,3],[2,3],[2,4]]) sage: findstat( [([g,h,i],[1,1,1])] ) --------------------------------------------------------------------------- ... ValueError: FindStat expects that every object occurs at most once.
There is no change to do that without your normalization procedure. I was suggesting only to replace the len(set(all_entries)) == len(all_entries)
by a short method to check that all_entries
is a duplicate-free list. That is a one-liner and does what we did before slower but for non-hashable objects. But it does also not achieve the normalization.
As long as the timings are okay (and I haven't seen any issues so far, but sending a zillion might be a problem) I am happy with your solution...
comment:75 in reply to: ↑ 73 Changed 7 years ago by
Replying to stumpc5:
Just a quick issue along the way:
sage: S = Permutations() sage: a,b,c = S([1,2,3]),S([1,3,2]),S([2,1,3]) sage: findstat( [([a],[1]),([b],[1])] ) a new statistic on Cc0001: PermutationsShouldn't that return an error as too little values are given?
Yes, and if you look closely you'll see already a doctest that doesn't know how to fail yet :-)
comment:76 Changed 7 years ago by
Replying to stumpc5:
Shouldn't that return an error as too little values are given?
I just checked: FindStat internally, it is allowed to send less than 3 entries, but it does not capture anything since only hits with 3 or more coincidences are returned. I think that's okay to leave. Anyway, I will add a check that not too little information is sent...
Am I right that you are not touching the file currently?
comment:77 Changed 7 years ago by
Before I do the change: is
# this might go wrong: try: assert data != [] except: raise ValueError("after discarding elements not in the range, and keeping less than %s values, nothing remained to send to FindStat." %FINDSTAT_MAX_VALUES)
any better than
if data == []: raise ValueError("after discarding elements not in the range, and keeping less than %s values, nothing remained to send to FindStat." %FINDSTAT_MAX_VALUES)
I would otherwise do
if len(data) < FINDSTAT_MIN_VALUES: raise ValueError("After discarding elements not in the range, too little (=%s) values remained to send to FindStat." %FINDSTAT_MIN_VALUES)
comment:78 Changed 7 years ago by
ad 0: no, I'm not going to touch the file.
ad 1: no, I like the second version (yours) much better.
ad 2: OK, except that your test is not correct, you need the sum of the lengths of the lists in data.
comment:79 Changed 7 years ago by
- Branch changed from u/mantepse/generating_function_in_findstat_interface to u/stumpc5/generating_function_in_findstat_interface
comment:80 Changed 7 years ago by
- Commit changed from 4d6e9eb840d59a6b2b3b0446b1162d19eb6dc893 to c41472f6e47c8b215079da04dfe85b52f91c1222
Branch pushed to git repo; I updated commit sha1. New commits:
c41472f | added a min values counter doctest
|
comment:81 Changed 7 years ago by
- Commit changed from c41472f6e47c8b215079da04dfe85b52f91c1222 to edc3e89bacf23558c6f1e443439536618dff8518
Branch pushed to git repo; I updated commit sha1. New commits:
edc3e89 | fixed some doctests, all tests pass now
|
comment:82 Changed 7 years ago by
Seems ready for me for now. I did a few tests on graphs and the standardization doesn't seem to influence things too much. Let's wait for a user case complaining about that timing...
comment:83 Changed 7 years ago by
I have a few more docfixes, can I merge and push?
comment:84 Changed 7 years ago by
- Branch changed from u/stumpc5/generating_function_in_findstat_interface to u/mantepse/generating_function_in_findstat_interface
comment:85 Changed 7 years ago by
- Commit changed from edc3e89bacf23558c6f1e443439536618dff8518 to 025e77400992c076fd2e978b9759ba2411cfc063
- Status changed from needs_info to needs_review
New commits:
025e774 | a few fixes to error messages
|
comment:86 Changed 7 years ago by
- Branch changed from u/mantepse/generating_function_in_findstat_interface to u/stumpc5/generating_function_in_findstat_interface
comment:87 Changed 7 years ago by
- Commit changed from 025e77400992c076fd2e978b9759ba2411cfc063 to 65fb91274c7cd520bf1fa352da6701528fd64414
- Status changed from needs_review to positive_review
comment:88 Changed 7 years ago by
- Status changed from positive_review to needs_work
merge conflict, wait for the next beta
comment:89 Changed 7 years ago by
Am I supposed to see the merge conflict somewhere?
comment:90 Changed 7 years ago by
It only touches findstat.py
so I guess the conflict is with #19314. I will pull 6.9
in a bit and then see if I can find the conflict. Let me know if you have solved it before...
comment:91 Changed 7 years ago by
OK, I won't touch it. Thanks!
comment:92 Changed 7 years ago by
- Commit changed from 65fb91274c7cd520bf1fa352da6701528fd64414 to 6c7099a5e0a4c036fd5c4fa8060a70898ef0b45e
Branch pushed to git repo; I updated commit sha1. New commits:
6c7099a | Merge branch 'develop' into t/19296/generating_function_in_findstat_interface
|
comment:93 Changed 7 years ago by
I cannot see any merge conflicts either when merging 6.9
into this branch. I might have to do with all our take-stuff-out-of-ticket-and-move-it-into-another-ticket-and-get-it-back-here thing, but I wouldn't know where it goes wrong.
For the record: the only file changed from 6.9
when merging this ticket is src/sage/databases/findstat.py
and the merge went through cleanly.
comment:94 Changed 7 years ago by
- Commit changed from 6c7099a5e0a4c036fd5c4fa8060a70898ef0b45e to 3bb57edb4bdf1cfe0951136781ab786185f0c20a
Branch pushed to git repo; I updated commit sha1. New commits:
3bb57ed | merged 6.10.rc0 and fixed conflicts
|
comment:95 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:96 Changed 7 years ago by
I merged the the newest develop and fixed the indeed occurring merge conflict. I usually do mistakes doing that (even though I try not to), so please have a brief look again before giving it a positive review.
comment:97 Changed 7 years ago by
Martin, could you have a quick look here and give it a positive review?
comment:98 Changed 7 years ago by
ping
comment:99 Changed 7 years ago by
- Milestone changed from sage-6.9 to sage-6.10
- Status changed from needs_review to needs_work
Don't use except:
, catch specific exceptions or use except Exception:
to catch all standard exceptions.
comment:100 Changed 7 years ago by
- Branch changed from u/stumpc5/generating_function_in_findstat_interface to u/mantepse/generating_function_in_findstat_interface
comment:101 Changed 7 years ago by
- Commit changed from 3bb57edb4bdf1cfe0951136781ab786185f0c20a to 72877fad8284b604978163ef10a54fa6421c17f1
- Description modified (diff)
- Status changed from needs_work to needs_review
comment:102 Changed 7 years ago by
- Commit changed from 72877fad8284b604978163ef10a54fa6421c17f1 to 1c83fe3962b1ef7dd698b7e1bab86561473d9d27
Branch pushed to git repo; I updated commit sha1. New commits:
1c83fe3 | tiny fixes
|
comment:103 Changed 7 years ago by
I'm giving the parts Christian wrote positive review.
comment:104 Changed 7 years ago by
- Reviewers changed from Martin Rubey to Martin Rubey, Christian Stump
comment:105 Changed 7 years ago by
- Status changed from needs_review to positive_review
looks good to me, and the patchbot is happy...
comment:106 Changed 7 years ago by
- Status changed from positive_review to needs_work
This is silly:
try: assert foo except AssertionError: raise ValueError(...)
it would be much better to write this is
if not foo: raise ValueError(...)
but really, I think you also want to catch TypeError
from the depth = int(depth)
call.
You should also add doctests for these bad inputs.
comment:107 Changed 7 years ago by
- Commit changed from 1c83fe3962b1ef7dd698b7e1bab86561473d9d27 to fd221b7260bd2f9fdd5864619a8346daa71b3d59
comment:108 Changed 7 years ago by
Do you like this version better? (I want to keep the argument checking of depth short and as one entity, that's why I did it this way.)
comment:109 Changed 7 years ago by
- Status changed from needs_work to positive_review
comment:110 Changed 7 years ago by
Fine for me.
Please remember that you should never use except:
, as it is usually the wrong thing to do (it also catches KeyboardInterrupt
for example).
comment:111 Changed 7 years ago by
- Status changed from positive_review to needs_work
Doctests must run without network access
sage -t --long src/sage/databases/findstat.py ********************************************************************** File "src/sage/databases/findstat.py", line 486, in sage.databases.findstat.FindStat.__call__ Failed example: findstat("Permutations", 1) Expected: Traceback (most recent call last): ... ValueError: The given arguments, Permutations and 1, cannot be used for a FindStat search. Got: <BLANKLINE> Traceback (most recent call last): File "/mnt/highperf/buildbot/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 "/mnt/highperf/buildbot/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.FindStat.__call__[3]>", line 1, in <module> findstat("Permutations", Integer(1)) File "sage/misc/lazy_import.pyx", line 383, in sage.misc.lazy_import.LazyImport.__call__ (/mnt/highperf/buildbot/slave/sage_git/build/src/build/cythonized/sage/misc/lazy_import.c:3593) return self._get_object()(*args, **kwds) File "/mnt/highperf/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/databases/findstat.py", line 613, in __call__ (collection, to_str) = get_collection(None, query_1) File "/mnt/highperf/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/databases/findstat.py", line 515, in get_collection collection = FindStatCollection(element) File "sage/misc/classcall_metaclass.pyx", line 326, in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (/mnt/highperf/buildbot/slave/sage_git/build/src/build/cythonized/sage/misc/classcall_metaclass.c:1239) return cls.classcall(cls, *args, **kwds) File "/mnt/highperf/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/databases/findstat.py", line 1861, in __classcall_private__ return FindStatCollections()(entry) File "sage/misc/classcall_metaclass.pyx", line 326, in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (/mnt/highperf/buildbot/slave/sage_git/build/src/build/cythonized/sage/misc/classcall_metaclass.c:1239) return cls.classcall(cls, *args, **kwds) File "sage/misc/cachefunc.pyx", line 1304, in sage.misc.cachefunc.WeakCachedFunction.__call__ (/mnt/highperf/buildbot/slave/sage_git/build/src/build/cythonized/sage/misc/cachefunc.c:8470) w = self.f(*args, **kwds) File "/mnt/highperf/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/structure/unique_representation.py", line 1021, in __classcall__ instance = typecall(cls, *args, **options) File "sage/misc/classcall_metaclass.pyx", line 493, in sage.misc.classcall_metaclass.typecall (/mnt/highperf/buildbot/slave/sage_git/build/src/build/cythonized/sage/misc/classcall_metaclass.c:1665) return (<PyTypeObject*>type).tp_call(cls, args, kwds) File "/mnt/highperf/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/databases/findstat.py", line 2357, in __init__ for j in json.load(urlopen(FINDSTAT_URL_DOWNLOADS_COLLECTIONS)): File "/mnt/highperf/buildbot/slave/sage_git/build/local/lib/python/urllib2.py", line 154, in urlopen return opener.open(url, data, timeout) File "/mnt/highperf/buildbot/slave/sage_git/build/local/lib/python/urllib2.py", line 431, in open response = self._open(req, data) File "/mnt/highperf/buildbot/slave/sage_git/build/local/lib/python/urllib2.py", line 449, in _open '_open', req) File "/mnt/highperf/buildbot/slave/sage_git/build/local/lib/python/urllib2.py", line 409, in _call_chain result = func(*args) File "/mnt/highperf/buildbot/slave/sage_git/build/local/lib/python/urllib2.py", line 1227, in http_open return self.do_open(httplib.HTTPConnection, req) File "/mnt/highperf/buildbot/slave/sage_git/build/local/lib/python/urllib2.py", line 1197, in do_open raise URLError(err) URLError: <urlopen error [Errno 110] Connection timed out> ********************************************************************** 1 item had failures: 1 of 6 in sage.databases.findstat.FindStat.__call__ [54 tests, 1 failure, 127.29 s]
comment:112 Changed 7 years ago by
- Commit changed from fd221b7260bd2f9fdd5864619a8346daa71b3d59 to 00dc9e56087a8e79f72d3848422878cef667264c
Branch pushed to git repo; I updated commit sha1. New commits:
04162f7 | Merge branch 'develop' of git://github.com/sagemath/sage into t/19296/generating_function_in_findstat_interface
|
00dc9e5 | Merge branch 'u/mantepse/generating_function_in_findstat_interface' of git://trac.sagemath.org/sage into t/19296/generating_function_in_findstat_interface
|
comment:113 Changed 7 years ago by
- Commit changed from 00dc9e56087a8e79f72d3848422878cef667264c to 15ec3561001bbb9f6372e4d245959f6a2adb470a
Branch pushed to git repo; I updated commit sha1. New commits:
15ec356 | fix test thanks to vbraun
|
comment:114 Changed 7 years ago by
- Status changed from needs_work to positive_review
Sorry about that, fixed.
comment:115 Changed 7 years ago by
- Branch changed from u/mantepse/generating_function_in_findstat_interface to 15ec3561001bbb9f6372e4d245959f6a2adb470a
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
first version for generating function and automated oeis search, still to be improved