Opened 3 years ago
Closed 3 years ago
#15351 closed defect (fixed)
import_statements sometimes fails
Reported by:  vdelecroix  Owned by:  vdelecroix 

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)  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 3 years ago by
comment:1 Changed 3 years ago by
 Description modified (diff)
Changed 3 years ago by
comment:2 Changed 3 years ago by
 Description modified (diff)
 Status changed from new to needs_review
comment:3 Changed 3 years ago by
 Description modified (diff)
 Status changed from needs_review to needs_work
comment:4 Changed 3 years ago by
 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 3 years ago by
 Description modified (diff)
comment:6 Changed 3 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:7 Changed 3 years ago by
comment:8 Changed 3 years ago by
 Commit changed from 51f7c545dc7e3f5867dc3548dd8d8258b8996dd5 to cd681db50acad79e5a4d6bd4d595d6d899e97613
comment:9 Changed 3 years ago by
 Commit changed from cd681db50acad79e5a4d6bd4d595d6d899e97613 to 84df0e328813a0d298020e128590861633436f2a
Branch pushed to git repo; I updated commit sha1. New commits:
84df0e3  few exceptions left !!

comment:10 Changed 3 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 3 years ago by
 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 3 years ago by
 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 followup: ↓ 14 Changed 3 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 in reply to: ↑ 13 Changed 3 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 3 years ago by
 Commit changed from 815320ea5e5f3dd58d195599e0da620e67906cc1 to 7873ad53d2a017232ac0d15d469898a074bc2b81
Branch pushed to git repo; I updated commit sha1. New commits:
7873ad5  Polish documentation

comment:16 Changed 3 years ago by
 Commit changed from 7873ad53d2a017232ac0d15d469898a074bc2b81 to 60485b907601f34c099c40cdd05841b8dea3c157
Branch pushed to git repo; I updated commit sha1. New commits:
60485b9  finalize

comment:17 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:18 Changed 3 years ago by
 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 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 3 years ago by
 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 3 years ago by
 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 3 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 3 years ago by
 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