Opened 12 months ago

Closed 7 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)

failed_import_statements (3.8 KB) - added by vdelecroix 12 months ago.
the list of import_statements that fail
fix_import_statements.patch (5.5 KB) - added by vdelecroix 12 months ago.
answer_sage_all (687 bytes) - added by vdelecroix 12 months ago.
the list of import_statements that answer "from sage.all import XXX"

Download all attachments as: .zip

Change History (25)

Changed 12 months ago by vdelecroix

the list of import_statements that fail

comment:1 Changed 12 months ago by vdelecroix

  • Description modified (diff)

Changed 12 months ago by vdelecroix

Changed 12 months ago by vdelecroix

the list of import_statements that answer "from sage.all import XXX"

comment:2 Changed 12 months ago by vdelecroix

  • Description modified (diff)
  • Status changed from new to needs_review

comment:3 Changed 12 months ago by vdelecroix

  • Description modified (diff)
  • Status changed from needs_review to needs_work

comment:4 Changed 12 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 12 months ago by vdelecroix

  • Description modified (diff)

comment:6 Changed 9 months ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:7 Changed 8 months ago by chapoton

Some related tickets : #14329 and #15333 (that needs review)

comment:8 Changed 7 months ago by git

  • Commit changed from 51f7c545dc7e3f5867dc3548dd8d8258b8996dd5 to cd681db50acad79e5a4d6bd4d595d6d899e97613

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

a810144Merge branch 'u/vdelecroix/improve_import_statements' of trac.sagemath.org:sage into 15351
4cca0b5better organization... no more error!
cd681dbbetter documentation

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:

84df0e3few 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:

815320eSimplify the organisation by splitting into different ones

comment:13 follow-up: 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 7 months ago by git

  • Commit changed from 815320ea5e5f3dd58d195599e0da620e67906cc1 to 7873ad53d2a017232ac0d15d469898a074bc2b81

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

7873ad5Polish documentation

comment:16 Changed 7 months ago by git

  • Commit changed from 7873ad53d2a017232ac0d15d469898a074bc2b81 to 60485b907601f34c099c40cdd05841b8dea3c157

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

60485b9finalize

comment:17 Changed 7 months ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:18 Changed 7 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:

c0f9d70trac #15351: Rebase on 6.2.beta5
76e56e2trac #15351: Reviewer's commit

comment:19 Changed 7 months ago by git

  • Commit changed from 76e56e25f15d1100b2aa827398616e6ff5756359 to b880d2d735a3651d06e83f40dfadd51df18bb2ee

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

b880d2dPut comment in the code and add more examples.

comment:20 Changed 7 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 7 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 7 months ago by vbraun

  • Branch changed from public/improve_import_statements to b880d2d735a3651d06e83f40dfadd51df18bb2ee
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.