Opened 7 years ago

Closed 7 years ago

#14499 closed enhancement (fixed)

Some cleaup in sage/combinat/design

Reported by: ncohen Owned by: sage-combinat
Priority: major Milestone: sage-5.10
Component: combinatorics Keywords:
Cc: dimpase, rbeezer Merged in: sage-5.10.beta2
Authors: Nathann Cohen Reviewers: Stefan van Zwam
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #14483 Stopgaps:

Description (last modified by Stefan)

As the title says.. Documentation fixes and reorganisation. We create a catalog "designs." similar to "groups", "graphs", etc.

Apply:

Attachments (6)

trac_14499-doctests.patch (5.0 KB) - added by ncohen 7 years ago.
trac_14499-db_internet.patch (6.9 KB) - added by ncohen 7 years ago.
trac_14499-catalog.patch (11.6 KB) - added by ncohen 7 years ago.
trac_14499.patch (43.7 KB) - added by ncohen 7 years ago.
trac_14499-deprecations.patch (2.4 KB) - added by ncohen 7 years ago.
trac_14499-review-change.patch (978 bytes) - added by Stefan 7 years ago.
Small change to docstring of block_design.py

Download all attachments as: .zip

Change History (23)

comment:1 Changed 7 years ago by ncohen

  • Status changed from new to needs_review

comment:2 Changed 7 years ago by ncohen

  • Description modified (diff)

Changed 7 years ago by ncohen

comment:3 Changed 7 years ago by ncohen

  • Description modified (diff)

Changed 7 years ago by ncohen

comment:4 Changed 7 years ago by ncohen

  • Cc dimpase rbeezer added

comment:5 Changed 7 years ago by ncohen

  • Dependencies set to #14483

comment:6 Changed 7 years ago by Stefan

  • Status changed from needs_review to needs_work

Shouldn't there be deprecation warnings for the moved functions?

You should be careful with importing stuff in the designs catalog. When typing

sage: designs.<tab>

I get the following entries that shouldn't be there: VectorSpace?, ZZ. You also see BlockDesign? and IncidenceStructureFromMatrix? in that, both of which are available elsewhere, and probably shouldn't be visible either. Look at the implementation of the "groups" catalog for a way to avoid this.

comment:7 Changed 7 years ago by ncohen

Oh. Fake modules. Rightright !

Nathann

comment:8 Changed 7 years ago by ncohen

Ahahaahah. I was a bit stuck with my 4 patches, as you pointed out a problem in the second one. Well, turns out I left all these methods where they were in the first place, and so the 4 patches become only two. Please do not look at it right now, for I still want to check a couple of things :-)

Nathann

comment:9 Changed 7 years ago by ncohen

  • Description modified (diff)

Changed 7 years ago by ncohen

comment:10 Changed 7 years ago by ncohen

  • Status changed from needs_work to needs_review

Ok, looks good again :-)

About deprecation warnings : which function do you have in mind ? If it is the one that downloads a design from internet then the problem is fixed as this function is not moved anymore, nor renamed. It is imported with a different name, though. If, on the other hand, you think of points_from_gap then...

Well. It's up to you. But its docstring seems to say that no one exactly knows why it is there, and it does not seem to do anything useful... It really looks like the kind of thing that has been forgotten a long time ago.

I can put a warning if you want, but it would mean that this thing will stay around for one year, nobody knowing exactly what it does... I personally think that we are better without it.

Have fuuuuuuuuun !

Nathann

comment:11 follow-up: Changed 7 years ago by Stefan

I just looked at this in the developer guide:

http://www.sagemath.org/doc/developer/coding_in_python.html#deprecation

It would apply to all of AffineGeometryDesign?, ProjectiveGeometryDesign?, WittDesign?, HadamardDesign?, BlockDesign_generic (well, maybe this one is trickier because it's a class that's still around), best_known_covering_design_www, points_from_gap.

I don't know how strictly the deprecation procedure is enforced (I'm still new to Sage development), but it seems common courtesy to me to warn current users of where these things have moved to. I'll be ok with not deprecating if an experienced Sage developer supports it too.

comment:12 in reply to: ↑ 11 Changed 7 years ago by ncohen

  • Status changed from needs_review to needs_work

Hellooooooooo !!

I don't know how strictly the deprecation procedure is enforced (I'm still new to Sage development), but it seems common courtesy to me to warn current users of where these things have moved to. I'll be ok with not deprecating if an experienced Sage developer supports it too.

As everywhere around the world, here is how the law works : somebody decides that one thing should appear in the law without getting everybody's assent, and then everybody has to conform to it, regardless of what they thought of the rule in the first place.

I personally totally hate that rule most of the times it is applied, and the day is near when I will refrain from sending a patch because of a rule that makes me do work that I find meaningless. Unfortunately for me, I want this patch to get in. So I will do what I believe to be bad work. And I hate compromission.

(I am sorry Stefan, for you do not deserve this. It's my battle against the world.)

Will do !

Nathann

Changed 7 years ago by ncohen

Changed 7 years ago by ncohen

comment:13 Changed 7 years ago by ncohen

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

Here it is ! It should be done by the book. I compromised again :-P

And it will stay there for one year... And be forgotten, before another ticket is opened to remove them.

I was actually quite surprised that a function like deprecated_callable_import existed somewhere !

Note that I also changed the first patch, as it is the one which removed points_from_gap. This method is not removed, but code is added to it (as it is the custom) to add the deprecation warning. Granted, I lost two hours of sleep and kilograms of good karma, but two guys on a weird continent whom I never met will not need to do a google search the next time they upgrade their version of Sage, even though the Google search would have led them to check out the new features while they were at it. And that's worth more than my physical health :-P

Nathann

Changed 7 years ago by Stefan

Small change to docstring of block_design.py

comment:14 Changed 7 years ago by Stefan

  • Description modified (diff)
  • Reviewers set to Stefan van Zwam

I had a tiny issue with the documentation; to save you work I added an extra patch. If someone (Nathann?) can OK that patch then I can give this ticket a positive review.

comment:15 Changed 7 years ago by ncohen

  • Status changed from needs_review to positive_review

Oh. That small. You could have asked, it would have been done quickly... :-)

Thank you for this review !

Nathann

comment:16 Changed 7 years ago by ncohen

Apply trac_14499.patch trac_14499-catalog.patch trac_14499-deprecations.patch trac_14499-review-change.patch

comment:17 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.10.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.