Opened 4 years ago

Closed 4 years ago

#26016 closed defect (fixed)

py3: miscellaneous fixes and cleanup to sage.sandpiles

Reported by: Erik Bray Owned by:
Priority: major Milestone: sage-8.4
Component: python3 Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 9c407d2 (Commits, GitHub, GitLab) Commit: 9c407d266046cc296bb6f99c000fd2f978f917e9
Dependencies: Stopgaps:

Status badges

Description

This fixes a few errors that occurred in Python 3, and in the process performs some other cleanup tasks that stood out in this module, though there's lots more cleaning up that could be done here.

This doesn't fix all the tests for this module on Python 3, as there are still some failures due to sets being displayed in different orders and things like that--but most of them seem trivial.

Change History (15)

comment:1 Changed 4 years ago by Erik Bray

Status: newneeds_review

comment:2 Changed 4 years ago by Julian Rüth

Could you also have a look at the pyflakes errors since you are cleaning up anyway?

comment:3 Changed 4 years ago by Julian Rüth

Status: needs_reviewneeds_work

comment:4 Changed 4 years ago by Erik Bray

Yes, certainly (there are probably a lot in that module).

comment:5 Changed 4 years ago by Erik Bray

Well, actually, it's only reporting one small issue. I guess however the patchbot is configured to run pyflakes it doesn't do much PEP-8 checking, which is probably a good thing because that would be overwhelming...

comment:6 Changed 4 years ago by Erik Bray

Status: needs_workneeds_review

As it turns out, I think the "unused variable" pyflakes is complaining about here is actually somewhat necessary, just to ensure that the singular ring it references doesn't get deleted or something. I'm going to leave it for now, unless there is a particular syntax we're using to mark a line as ignored by linters. I'm not sure we have a specific syntax for that (most linters do but I'm not sure if we're actually consistently doing that yet...)

comment:7 Changed 4 years ago by Julian Rüth

There's no syntax for that afaik.

comment:8 Changed 4 years ago by Julian Rüth

Reviewers: Julian Rüth
Status: needs_reviewneeds_work

I think that you need a doctest for the method that you move to the top level. (The coverage plugin seems to be rightly complaining.)

comment:9 Changed 4 years ago by Erik Bray

Status: needs_workneeds_review

I explicitly didn't because it's already covered by the tests for the methods that wrap it.

Version 0, edited 4 years ago by Erik Bray (next)

comment:10 Changed 4 years ago by Julian Rüth

Status: needs_reviewneeds_work

I'm sorry but the rules say that you need one "An EXAMPLES block for examples. This is not optional." http://doc.sagemath.org/html/en/developer/coding_basics.html.

Actually I wonder if we should drop all these help methods. We usually don't have them. How are sandpiles different?

comment:11 Changed 4 years ago by Erik Bray

Status: needs_workneeds_review

*sigh* I've had this argument before. At the very least if we're going to have to have this argument again can you leave the status in place?

These functions already have an EXAMPLES block. This is just refactoring existing code and moving it verbatim elsewhere. It is wrapped directly by these functions which provide the usage examples and test coverage. The fact that doing so technically involves a new function is an implementation detail. See a previous discussion where we already settled this: https://groups.google.com/d/msg/sage-devel/ptdNkphTa1g/KfanYQGkBQAJ

Last edited 4 years ago by Erik Bray (previous) (diff)

comment:12 Changed 4 years ago by Erik Bray

FWIW, perhaps if nothing else those guidelines need to be revisited or slightly reworded. I think mostly as written it's good, but it's kind of aimed more towards contributions of new code, specifically end-user APIs (though some have argued that examples can be valuable for underscored internal functions, and I agree that is often the case, though not always).

But the guidelines don't really make as much sense in the context of refactoring existing code or other miscellaneous skunkworks of that sort.

As you know, I'm also working on getting actual line profiled test coverage working for Sage, which in principle is more valuable. It's a bit tricky though because all the relevant Cython modules have to be compiled with line profiling enabled which makes them much slower. I suspect it will be more useful for getting coverage on individual modules. It will probably also be nice in conjunction with --short.

comment:13 in reply to:  10 Changed 4 years ago by Erik Bray

Replying to saraedum:

Actually I wonder if we should drop all these help methods. We usually don't have them. How are sandpiles different?

I do actually rather like these help() methods. I have no idea why they just happen to be on classes for sandpiles. I guess whoever implemented this module wanted it--or maybe it came from a separate package that wasn't originally part of Sage.

My inclination is almost to move the _sandpile_help() function to a generic method on SageObject (in which case then yes, it would get an EXAMPLES section in the docstring), and have all these classes inherit from SageObject.

comment:14 Changed 4 years ago by Frédéric Chapoton

Reviewers: Julian RüthFrédéric Chapoton
Status: needs_reviewpositive_review

ok..

comment:15 Changed 4 years ago by Volker Braun

Branch: u/embray/python3/sage-sandpiles/misc9c407d266046cc296bb6f99c000fd2f978f917e9
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.