Opened 11 months ago
Closed 6 months ago
#15351 closed defect (fixed)
import_statements sometimes fails
Reported by: | vdelecroix | Owned by: | vdelecroix |
---|---|---|---|
Priority: | major | Milestone: | sage-6.2 |
Component: | misc | Keywords: | |
Cc: | Merged in: | ||
Authors: | Vincent Delecroix | Reviewers: | Nathann Cohen |
Report Upstream: | N/A | Work issues: | |
Branch: | b880d2d (Commits) | Commit: | b880d2d735a3651d06e83f40dfadd51df18bb2ee |
Dependencies: | Stopgaps: |
Description (last modified by vdelecroix)
Sometimes import_statements fails in given a working answer (pi, CIF, ...). See the complete list in failed_import_statements.
We fix the bug and the solution proposed contains a new function *which_import_statements_fail* that build the list of name in sage.all for which the answer of import_statements fail to import it. It also fixes many input: only one import statement fails "maxima_calculus" and with 62 occurences we obtain a "from sage.all import XXX" (see the precise list in answer_sage_all).
Attachments (3)
Change History (25)
Changed 11 months ago by vdelecroix
comment:1 Changed 11 months ago by vdelecroix
- Description modified (diff)
Changed 11 months ago by vdelecroix
Changed 11 months ago by vdelecroix
the list of import_statements that answer "from sage.all import XXX"
comment:2 Changed 11 months ago by vdelecroix
- Description modified (diff)
- Status changed from new to needs_review
comment:3 Changed 11 months ago by vdelecroix
- Description modified (diff)
- Status changed from needs_review to needs_work
comment:4 Changed 11 months ago by vdelecroix
- Branch set to u/vdelecroix/improve_import_statements
- Commit set to 51f7c545dc7e3f5867dc3548dd8d8258b8996dd5
- Status changed from needs_work to needs_review
New commits:
[changeset:51f7c54] | all test pass |
[changeset:b4f1af2] | initial modification |
comment:5 Changed 11 months ago by vdelecroix
- Description modified (diff)
comment:6 Changed 8 months ago by vbraun_spam
- Milestone changed from sage-6.1 to sage-6.2
comment:7 Changed 7 months ago by chapoton
comment:8 Changed 7 months ago by git
- Commit changed from 51f7c545dc7e3f5867dc3548dd8d8258b8996dd5 to cd681db50acad79e5a4d6bd4d595d6d899e97613
comment:9 Changed 7 months ago by git
- Commit changed from cd681db50acad79e5a4d6bd4d595d6d899e97613 to 84df0e328813a0d298020e128590861633436f2a
Branch pushed to git repo; I updated commit sha1. New commits:
84df0e3 | few exceptions left !! |
comment:10 Changed 7 months ago by vdelecroix
I fixed (checking one by one) all the exceptions in the linked file (except the is_* stuff)... and this is now ready for review !!
It is much more robust but it would be marvellous to detect deprecated stuff (there is a note about that in the documentation of the function).
comment:11 Changed 7 months ago by ncohen
- Status changed from needs_review to needs_work
Helloooooooooooooooooooooooo !!!
Some remarks:
1) Affected ? That's only correct in french I thing. Do you want to translate "variable assignment" ? But I believe that in this context messages like "x is defined by <...>" or "x is equal to <...>" would be clearer.
2) And one can actually check that FareySymbol and sum are aliases. Would be better with :func:`FareySymbol` for the html doc.
3) If there is an option to display the import line for lazy functions then the output of this function is meant to be copy/pasted. Thus, shouldn't the Warning line begin with a # ? Otherwise it is impossible to copy/paste it into Python code.
4) use it has a name : as a name.
5) This function is awful. It may be nice to split it into identify_module, identify_object_from_module, identify_by_search_through_modules, identify_object_from_source_code, etc ?... Would make the code a bit clearer :-P
6) names_pattern = dict((i,j) for i,j in whatever) can be rewritten as {i:j for i,v in whatever}. Would make your names_pattern line clearer.
7) at the beginig
8) Why this print_or_update function ?
I have not understood everything of the code yet, but fixing the things above might help ^^;
Nathann
comment:12 Changed 7 months ago by git
- Commit changed from 84df0e328813a0d298020e128590861633436f2a to 815320ea5e5f3dd58d195599e0da620e67906cc1
Branch pushed to git repo; I updated commit sha1. New commits:
815320e | Simplify the organisation by splitting into different ones |
comment:13 follow-up: ↓ 14 Changed 7 months ago by vdelecroix
Hi,
Thanks so much for telling me to rewrite it ! Now it is simpler and cleaner and I hope you will understand everything.
I do have two things to solve before getting this in a needs-review status
- try to not load extra modules in the function find_objects_from_name (see the thread on sage-devel)
- possibly move the functions find_objects_from_name and find_object_modules in sage.misc.sageinspect
See you soooooooooooon
comment:14 in reply to: ↑ 13 Changed 7 months ago by ncohen
Yoooooo !!
Thanks so much for telling me to rewrite it ! Now it is simpler and cleaner and I hope you will understand everything.
I do have two things to solve before getting this in a needs-review status
Okayyyy.
See you soooooooooooon
Are you coming to Paris ? I am not there right now, but in Brussels. And I should move to Lieges next !
Nathann
comment:15 Changed 6 months ago by git
- Commit changed from 815320ea5e5f3dd58d195599e0da620e67906cc1 to 7873ad53d2a017232ac0d15d469898a074bc2b81
Branch pushed to git repo; I updated commit sha1. New commits:
7873ad5 | Polish documentation |
comment:16 Changed 6 months ago by git
- Commit changed from 7873ad53d2a017232ac0d15d469898a074bc2b81 to 60485b907601f34c099c40cdd05841b8dea3c157
Branch pushed to git repo; I updated commit sha1. New commits:
60485b9 | finalize |
comment:17 Changed 6 months ago by vdelecroix
- Status changed from needs_work to needs_review
comment:18 Changed 6 months ago by ncohen
- Branch changed from u/vdelecroix/improve_import_statements to public/improve_import_statements
- Commit changed from 60485b907601f34c099c40cdd05841b8dea3c157 to 76e56e25f15d1100b2aa827398616e6ff5756359
Hello...
Really I tried, and I don't get a word of what the main function does. I don't get it, I don't get how it does anything, I don't even get at every moment which variables can be assumed to be defined, what is left to be found, I don't get the flow of it. Sorry, I tried and I just don't get it.
If you cannot split it into subfunctions further, please add comments to say from time to time "This has been defined, now we look for this". I just don't get it, all I can do right now is check that three consecutive lines make sense, and that's not doing a very good review job.
I add a small commit with a couple of unimportant modifications.
Some remarks:
- Why is exclude_pattern overwritten when module is None ?
- "Here we test some instances::" is followed by nothing
- Why should calling import_statement on a module raise an exception ? What if I do not know where graph_decompositions is located ?
sage: import_statements("graph_decompositions") ... ValueError: the object '<module 'sage.graphs.graph_decompositions' from '/home/ncohen/.Sage/local/lib/python2.7/site-packages/sage/graphs/graph_decompositions/__init__.pyc'>' is a module
More importantly, what should the following code do : raise an exception of continue ?
raise ValueError("the object '%s' is a module"%obj) continue
Nathann
New commits:
c0f9d70 | trac #15351: Rebase on 6.2.beta5 |
76e56e2 | trac #15351: Reviewer's commit |
comment:19 Changed 6 months ago by git
- Commit changed from 76e56e25f15d1100b2aa827398616e6ff5756359 to b880d2d735a3651d06e83f40dfadd51df18bb2ee
Branch pushed to git repo; I updated commit sha1. New commits:
b880d2d | Put comment in the code and add more examples. |
comment:20 Changed 6 months ago by ncohen
- Reviewers set to Nathann Cohen
- Status changed from needs_review to positive_review
Okay, still not an easy read but... better T_T
Thanks !
Nathann
comment:21 Changed 6 months ago by vdelecroix
Hello,
Really I tried, and I don't get a word of what the main function does.
[...]
Ok. I had many comments into the code, especially about what the variables are.
- Why is exclude_pattern overwritten when module is None ?
Because you can crash Sage (really):
sage: from sage.misc.dev_tools import load_submodules sage: load_submodules(sage.libs) load sage.libs.coxeter3.coxeter_group...failed load sage.libs.cremona.homspace... suceeded ... load sage.libs.singular.groebner_strategy... suceeded sage: quit # BOOOOOM
- "Here we test some instances::" is followed by nothing
I removed it.
- Why should calling import_statement on a module raise an exception ? What if I do not know where graph_decompositions is located ?
sage: import_statements("graph_decompositions") ... ValueError: the object '<module 'sage.graphs.graph_decompositions' from '/home/ncohen/.Sage/local/lib/python2.7/site-packages/sage/graphs/graph_decompositions/__init__.pyc'>' is a module
Fixed.
Vincent
comment:22 Changed 6 months ago by vbraun
- Branch changed from public/improve_import_statements to b880d2d735a3651d06e83f40dfadd51df18bb2ee
- Resolution set to fixed
- Status changed from positive_review to closed
the list of import_statements that fail