Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#18107 closed enhancement (fixed)

The codes collection should describe how to import it as a real module

Reported by: jsrn Owned by:
Priority: major Milestone: sage-6.6
Component: coding theory Keywords: sd66
Cc: dlucas, ncohen, vdelecroix Merged in:
Authors: Johan Sebastian Rosenkilde Nielsen Reviewers: Nathann Cohen, Volker Braun
Report Upstream: N/A Work issues:
Branch: 28a26ea (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jsrn)

Currently, most coding theory-related stuff is hidden behind codes.<tab>. But it is done in a way which does not allow writing

   from codes import *

whenever you're working a lot with coding theory. Instead, one has to do

   from sage.coding.codes_catalog import *

This could be mentioned in the doc of codes.

Change History (30)

comment:1 Changed 7 years ago by jsrn

  • Keywords sagedays66 added

comment:2 Changed 7 years ago by vdelecroix

  • Keywords sd66 added; sagedays66 removed

comment:3 Changed 7 years ago by vdelecroix

See also the related #16393 (especially comment:6)

comment:4 Changed 7 years ago by jsrn

  • Branch set to u/jsrn/18107_coding_module

comment:5 Changed 7 years ago by jsrn

  • Commit set to 1625674fd698897f541645a9c1cc310ed411d424
  • Status changed from new to needs_review

New commits:

9613f8bwip making module importing work. Currently experimenting with
1625674Some code comments

comment:6 Changed 7 years ago by jsrn

My implementation retains the lazy import of codes. I have no idea if/how to make a doc-test for this :-S

comment:7 Changed 7 years ago by git

  • Commit changed from 1625674fd698897f541645a9c1cc310ed411d424 to 00ca11cb897c35a16a9d15918dcf8b6deff7b676

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

00ca11cSimpler way to make available as module, with less magick

comment:8 Changed 7 years ago by ncohen

Helloooooo Johan,

This looks good to go, except for a couple of things:

  • Could you remove the commented line `#import codes_catalog as codes`?
  • There is a doubly commented line `# # Make codes available as "from codes import *"`
  • The comment in codes_catalog explain 'how' all.py makes the from codes import * syntax available, and I feel that it is not the right place to do that. It is not very important though, just a remark.

Nathann

comment:9 Changed 7 years ago by git

  • Commit changed from 00ca11cb897c35a16a9d15918dcf8b6deff7b676 to ac67d8590036d264587c7348bde9e095ef9cbc53

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

ac67d85Fixes to commentaries

comment:10 Changed 7 years ago by git

  • Commit changed from ac67d8590036d264587c7348bde9e095ef9cbc53 to f4604893e1757420d61372d22f33c900ca6a316c

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

f460489revert some earlier changes

comment:11 Changed 7 years ago by jsrn

Ok, so the total changes turned out to be quite minimal :-)

comment:12 Changed 7 years ago by jsrn

  • Authors set to Johan S. R. Nielsen

comment:13 Changed 7 years ago by ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_review to positive_review

Yooooooooooooo !

The code looks good -> positive review.

About your branch: as you can see there are 5 commits, most of which undo what the others add. There is a very useful command in git which is 'git rebase -i': it lets you 'merge' several commits into one.

https://robots.thoughtbot.com/git-interactive-rebase-squash-amend-rewriting-history

You may also find git commit --amend useful.

No problem for this branch: let's get it in as it is. I only give those links as they did prove helpful in my own experience to make easier-to-read branches (even a posteriori, i.e. after all code has been written).

Cheers,

Nathann

comment:14 follow-up: Changed 7 years ago by jsrn

Ok, thanks for the tips Nathann. I didn't really know how the community felt about rewriting history. If I do a rebase after pushing to the public branch (which was the case here), I run the risk (afaik) of breaking other people's synchronisation to the branch (i.e. you). That could cause some annoyances. Before doing any public pushes, it can make complete sense to clean up history, as you say. I use "--amend" all the time ;-) (through magit in Emacs btw).

comment:15 in reply to: ↑ 14 Changed 7 years ago by ncohen

Yoooooooo !

Ok, thanks for the tips Nathann. I didn't really know how the community felt about rewriting history. If I do a rebase after pushing to the public branch (which was the case here), I run the risk (afaik) of breaking other people's synchronisation to the branch (i.e. you). That could cause some annoyances. Before doing any public pushes, it can make complete sense to clean up history, as you say. I use "--amend" all the time ;-) (through magit in Emacs btw).

Well, personally I do not mind. Most of the time you know rathe well whether anybody is already using your commits and there are not many risks anyway. Plus in case or problems (=I never met any) it is still straightforward to cherry-pick commits from the old branch to the new. On the other hand, I prefer to give reviewers a better picture of what the code does, plus the branch that will actually become part of Sage will be cleaner as a result!

Have fuuuuuuuuun,

Nathann

comment:16 Changed 7 years ago by vbraun

  • Status changed from positive_review to needs_work

thats not really your fault but you trigger a bug:

sage -t --long --warn-long 28.8 src/sage/misc/dev_tools.py
**********************************************************************
File "src/sage/misc/dev_tools.py", line 233, in sage.misc.dev_tools.find_objects_from_name
Failed example:
    dt.find_objects_from_name('FareySymbol')
Exception raised:
    Traceback (most recent call last):
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 496, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 858, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.misc.dev_tools.find_objects_from_name[1]>", line 1, in <module>
        dt.find_objects_from_name('FareySymbol')
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/misc/dev_tools.py", line 267, in find_objects_from_name
        for smodule_name, smodule in sys.modules.iteritems():
    RuntimeError: dictionary changed size during iteration

iteration over sys.modules should first get all keys and then iterate.

comment:17 Changed 7 years ago by vbraun

Also, namespacing modules is one of the great features of Python. What happens if somebody has a codes Python module that he wants to use in Sage?

comment:18 Changed 7 years ago by jsrn

Hi Volker,

I fixed the bug you found.

Regarding your other comment, I would recommend that user to import his codes module from a sub-module, like import mystuff.codes as mycodes. Sage injects loads of functions into the global namespace. Injecting modules as well is a natural next-step, in my opinion, once the global number of functions is high enough. That requires, as you point out, in certain circumstances a very slight annoyance on the side of users having their own modules.

I don't think a good solution is to keep codes, and other "collections of semi-global functions", as objects, thereby missing out on module functionality.

comment:19 Changed 7 years ago by git

  • Commit changed from f4604893e1757420d61372d22f33c900ca6a316c to ad8a74f57121d8df1d40765330129a520f4f975f

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

ad8a74fFix bug in find_objects_from_name related to lazy loading of modules

comment:20 Changed 7 years ago by jsrn

  • Status changed from needs_work to needs_review

comment:21 follow-up: Changed 7 years ago by ncohen

Is there a reason why you didn't just replace 'iteritems' with 'items' ?

Nathann

comment:22 Changed 7 years ago by vbraun

Note that in Python3 dict.items is like dict.iteritems in Python2.

Having said that, I don't like the clobbering of non-sage namespace. Whats wrong with from sage.coding.all import *?

Last edited 7 years ago by vbraun (previous) (diff)

comment:23 in reply to: ↑ 21 Changed 7 years ago by jsrn

Replying to ncohen:

Is there a reason why you didn't just replace 'iteritems' with 'items' ?

Nathann

I didn't think of it.

Having said that, I don't like the clobbering of non-sage namespace. Whats wrong with from sage.coding.all import *?

We're doing a selection in codes (as is done in the other module-like objects) such that it is the "user-level" classes which will be in codes, while much-less-frequently-accessed classes should be accessible in sage.coding.all. If you import sage.coding.all you'll get a lot of stuff you probably weren't interested in. Furthermore, it's not very logical from a user-point of view. "Hmm, there's a nice module codes that I'm using all the time. I think I'll import it so that I don't have to type codes. everywhere. What? I have to import a completely different path to do this? Why?"

comment:24 Changed 7 years ago by vbraun

Note that module imports can not be shadowed by variables:

$ sage -python
>>> sage = 1
>>> from sage.all import sin
>>> type(sin)
<class 'sage.functions.trig.Function_sin'>

So its perfectly fine to have an object bound to a variable named codes but its bad to have a module codes (without namespace), and it would be a terrible design pattern to litter the module names space with all kinds of object catalogs.

Just make your own sage.coding.codes module from which to import your selection of codes.

comment:25 Changed 7 years ago by jsrn

  • Description modified (diff)
  • Summary changed from The codes collection should be a real module to The codes collection should describe how to import it as a real module

I don't think I fully see why littering the global variable namespace is fine, but littering the module namespace is bad.

But I also don't care enough about this to discuss it at length. I've rolled back the changes and instead inserted a note in the doc of codes on how to import it.

comment:26 Changed 7 years ago by git

  • Commit changed from ad8a74f57121d8df1d40765330129a520f4f975f to 28a26eac05663eabf6d79d93eb512082fde27ba8

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

dedccacRoll back all changes
28a26eaInsert note about how to import codes

comment:27 Changed 7 years ago by vbraun

  • Reviewers changed from Nathann Cohen to Nathann Cohen, Volker Braun
  • Status changed from needs_review to positive_review

Global variables in the repl are a tradeoff. Sure, globals are usually bad (which is why you have to explicitly import things in the Sage library code). But then its the commandline, so we want to have a useful set of defaults. The important fact is: you can always overwrite stuff in the global variable namespace with stuff from your preferred module and everything works:

$ python
>>> list = 123      # why not
>>> list([1,2])     # of course that doesn't work then any more
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'int' object is not callable

>>> from __builtin__ import list    # get the old list back
>>> list([1,2])
[1, 2]
Last edited 7 years ago by vbraun (previous) (diff)

comment:28 Changed 7 years ago by vbraun

  • Branch changed from u/jsrn/18107_coding_module to 28a26eac05663eabf6d79d93eb512082fde27ba8
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:29 Changed 7 years ago by kcrisman

  • Authors changed from Johan S. R. Nielsen to Johan Sebastian Rosenkilde Nielsen, Luis Felipe Tabera Alonso
  • Commit 28a26eac05663eabf6d79d93eb512082fde27ba8 deleted

comment:30 Changed 7 years ago by kcrisman

  • Authors changed from Johan Sebastian Rosenkilde Nielsen, Luis Felipe Tabera Alonso to Johan Sebastian Rosenkilde Nielsen
Note: See TracTickets for help on using tickets.