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

[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.

[vdelecroix]

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

[jsrn]

New commits:

​9613f8b	wip making module importing work. Currently experimenting with
​1625674	Some code comments

[jsrn]

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

[git]

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

​00ca11c

Simpler way to make available as module, with less magick

[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

[git]

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

​ac67d85

Fixes to commentaries

[git]

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

​f460489

revert some earlier changes

[jsrn]

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

[ncohen]

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

[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).

[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

[vbraun]

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.

[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?

[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.

[git]

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

​ad8a74f

Fix bug in find_objects_from_name related to lazy loading of modules

[ncohen]

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

Nathann

[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 *?

[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?"

[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.

[jsrn]

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.

[git]

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

​dedccac	Roll back all changes
​28a26ea	Insert note about how to import codes

[vbraun]

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]