Opened 9 years ago
Closed 9 years ago
#15351 closed defect (fixed)
import_statements sometimes fails
Reported by:  Vincent Delecroix  Owned by:  Vincent Delecroix 

Priority:  major  Milestone:  sage6.2 
Component:  misc  Keywords:  
Cc:  Merged in:  
Authors:  Vincent Delecroix  Reviewers:  Nathann Cohen 
Report Upstream:  N/A  Work issues:  
Branch:  b880d2d (Commits, GitHub, GitLab)  Commit:  b880d2d735a3651d06e83f40dfadd51df18bb2ee 
Dependencies:  Stopgaps: 
Description (last modified by )
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 9 years ago by
Attachment:  failed_import_statements added 

comment:1 Changed 9 years ago by
Description:  modified (diff) 

Changed 9 years ago by
Attachment:  fix_import_statements.patch added 

Changed 9 years ago by
Attachment:  answer_sage_all added 

the list of import_statements that answer "from sage.all import XXX"
comment:2 Changed 9 years ago by
Description:  modified (diff) 

Status:  new → needs_review 
comment:3 Changed 9 years ago by
Description:  modified (diff) 

Status:  needs_review → needs_work 
comment:4 Changed 9 years ago by
Branch:  → u/vdelecroix/improve_import_statements 

Commit:  → 51f7c545dc7e3f5867dc3548dd8d8258b8996dd5 
Status:  needs_work → needs_review 
New commits:
[changeset:51f7c54]  all test pass 
[changeset:b4f1af2]  initial modification 
comment:5 Changed 9 years ago by
Description:  modified (diff) 

comment:6 Changed 9 years ago by
Milestone:  sage6.1 → sage6.2 

comment:8 Changed 9 years ago by
Commit:  51f7c545dc7e3f5867dc3548dd8d8258b8996dd5 → cd681db50acad79e5a4d6bd4d595d6d899e97613 

comment:9 Changed 9 years ago by
Commit:  cd681db50acad79e5a4d6bd4d595d6d899e97613 → 84df0e328813a0d298020e128590861633436f2a 

Branch pushed to git repo; I updated commit sha1. New commits:
84df0e3  few exceptions left !!

comment:10 Changed 9 years ago by
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 9 years ago by
Status:  needs_review → 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 9 years ago by
Commit:  84df0e328813a0d298020e128590861633436f2a → 815320ea5e5f3dd58d195599e0da620e67906cc1 

Branch pushed to git repo; I updated commit sha1. New commits:
815320e  Simplify the organisation by splitting into different ones

comment:13 followup: 14 Changed 9 years ago by
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 needsreview status
 try to not load extra modules in the function
find_objects_from_name
(see the thread on sagedevel)  possibly move the functions
find_objects_from_name
andfind_object_modules
insage.misc.sageinspect
See you soooooooooooon
comment:14 Changed 9 years ago by
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 needsreview 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 9 years ago by
Commit:  815320ea5e5f3dd58d195599e0da620e67906cc1 → 7873ad53d2a017232ac0d15d469898a074bc2b81 

Branch pushed to git repo; I updated commit sha1. New commits:
7873ad5  Polish documentation

comment:16 Changed 9 years ago by
Commit:  7873ad53d2a017232ac0d15d469898a074bc2b81 → 60485b907601f34c099c40cdd05841b8dea3c157 

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

comment:17 Changed 9 years ago by
Status:  needs_work → needs_review 

comment:18 Changed 9 years ago by
Branch:  u/vdelecroix/improve_import_statements → public/improve_import_statements 

Commit:  60485b907601f34c099c40cdd05841b8dea3c157 → 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 isNone
?  "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 wheregraph_decompositions
is located ?sage: import_statements("graph_decompositions") ... ValueError: the object '<module 'sage.graphs.graph_decompositions' from '/home/ncohen/.Sage/local/lib/python2.7/sitepackages/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 9 years ago by
Commit:  76e56e25f15d1100b2aa827398616e6ff5756359 → 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 9 years ago by
Reviewers:  → Nathann Cohen 

Status:  needs_review → positive_review 
Okay, still not an easy read but... better T_T
Thanks !
Nathann
comment:21 Changed 9 years ago by
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 isNone
?
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 wheregraph_decompositions
is located ?sage: import_statements("graph_decompositions") ... ValueError: the object '<module 'sage.graphs.graph_decompositions' from '/home/ncohen/.Sage/local/lib/python2.7/sitepackages/sage/graphs/graph_decompositions/__init__.pyc'>' is a module
Fixed.
Vincent
comment:22 Changed 9 years ago by
Branch:  public/improve_import_statements → b880d2d735a3651d06e83f40dfadd51df18bb2ee 

Resolution:  → fixed 
Status:  positive_review → closed 
the list of import_statements that fail