Opened 7 months ago

Closed 5 months ago

#26016 closed defect (fixed)

py3: miscellaneous fixes and cleanup to sage.sandpiles

Reported by: embray 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) Commit: 9c407d266046cc296bb6f99c000fd2f978f917e9
Dependencies: Stopgaps:

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 7 months ago by embray

  • Status changed from new to needs_review

comment:2 Changed 7 months ago by saraedum

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

comment:3 Changed 7 months ago by saraedum

  • Status changed from needs_review to needs_work

comment:4 Changed 7 months ago by embray

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

comment:5 Changed 7 months ago by embray

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 7 months ago by embray

  • Status changed from needs_work to needs_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 7 months ago by saraedum

There's no syntax for that afaik.

comment:8 Changed 7 months ago by saraedum

  • Reviewers set to Julian Rüth
  • Status changed from needs_review to needs_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 7 months ago by embray

  • Status changed from needs_work to needs_review

I explicitly didn't because it's already covered by the tests for the methods that wrap it. It's not really a new method so much as refactoring many duplicate ones into one place.

Last edited 7 months ago by embray (previous) (diff)

comment:10 follow-up: Changed 7 months ago by saraedum

  • Status changed from needs_review to needs_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 6 months ago by embray

  • Status changed from needs_work to needs_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 6 months ago by embray (previous) (diff)

comment:12 Changed 6 months ago by embray

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 6 months ago by embray

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 5 months ago by chapoton

  • Reviewers changed from Julian Rüth to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok..

comment:15 Changed 5 months ago by vbraun

  • Branch changed from u/embray/python3/sage-sandpiles/misc to 9c407d266046cc296bb6f99c000fd2f978f917e9
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.