Opened 3 years ago

Closed 3 years ago

#19941 closed defect (fixed)

Rename rings.finite_rings.constructor to finite_field_constructor

Reported by: ncohen Owned by:
Priority: major Milestone: sage-7.1
Component: finite rings Keywords:
Cc: roed, jdemeyer Merged in:
Authors: Nathann Cohen Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 25a961f (Commits) Commit: 25a961f6e41e97bcebfab2057749b676eb8002c6
Dependencies: Stopgaps:

Description

As the title says. This is associated with the following sage-devel thread:

https://groups.google.com/d/topic/sage-devel/S1k8HzZUBIs/discussion

This ticket is in conflict with #17569, but that ticket is in needs_work, and it is apparently a bit hard to merge/rebase two branches when one touches a file that the other renames.

Whichever of the two gets a positive review first, I will handle the merge with the other.

Nathann

Change History (17)

comment:1 Changed 3 years ago by ncohen

  • Branch set to public/19941
  • Commit set to 944735b847af910a17e6e4dafb771ef83382cf54
  • Status changed from new to needs_review

New commits:

944735btrac #19941: Rename rings.finite_rings.constructor to finite_field_constructor

comment:2 Changed 3 years ago by git

  • Commit changed from 944735b847af910a17e6e4dafb771ef83382cf54 to 7e1852a6e5d477c14a62631128e83618e860d9a6

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a486ef5trac #19941: Rename rings.finite_rings.constructor to finite_field_constructor
7e1852atrac #19941: new 'constructor.py' file

comment:3 Changed 3 years ago by git

  • Commit changed from 7e1852a6e5d477c14a62631128e83618e860d9a6 to 42718e8d42381ba299eae472417402719577795e

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

961c77etrac #19941: Rename rings.finite_rings.constructor to finite_field_constructor
42718e8trac #19941: new 'constructor.py' file

comment:4 follow-up: Changed 3 years ago by chapoton

the change to index.rst seems not to be correct..

comment:5 follow-up: Changed 3 years ago by roed

I don't agree with this change, for two reasons.

First, I intend to implement finite rings that are not finite fields, which will be used as quotients of p-adic extension fields. If the change proposed in this ticket is made then we will need to either create another file for the constructors of such rings, or to suffer a situation analogous to that derided in the thread, where complex lazy fields are defined in a file real_lazy.py.

Second, I don't see any benefit to making this change. Finite fields are examples of finite rings; what's wrong with the current name?

comment:6 in reply to: ↑ 5 Changed 3 years ago by ncohen

Second, I don't see any benefit to making this change. Finite fields are examples of finite rings; what's wrong with the current name?

I have less and less patience for people playing dumb.

Would a file named 'object.pyx' still get no reaction from you? Finite Rings are objects too. It is much too general and thus becomes misleading: that's what is wrong.

Look at the filename: it tells that you can expect the file contains *the* constructor of finite rings (there is not even a plural to constructor). Do you see FiniteRing inside? No. IntegerModRing (also a ring) is not even inside, it sits in its own file, and neither are other finite rings. Inside of that file, what you expect to see is a class named FiniteRing, and absolutely nothing else. Why would you object to having the constructor of finite fields in a file that bears its name? Do you also object to having IntegerModRing in integer_mod_ring.py? If not, by which sorcery?

Inside of rings/finite_rings/ you have 5 files in finite_field_*. How odd would it be for the file containing their main constructor to have the same prefix?

First, I intend to implement finite rings that are not finite fields,

And you will be welcome to create a new file for them when you will get to work.

Nathann

comment:7 follow-up: Changed 3 years ago by davidloeffler

Nathann: You are out of order, and your comments on this ticket and the associated sage_devel thread fall far below the standard of conduct appropriate for Sage contributors.

David Roe is one of Sage's most valued developers and he has worked tirelessly on Sage's code for finite and p-adic fields. You, on the other hand, clearly know nothing about the content of this file, and thus you are reduced to making aggressive, mocking comments about its name, which is invisible to most Sage users anyway. You may as well write to the authors of a paper in a top journal telling them that you don't like the title they've given their paper.

Don't you have anything better to do with your time?

comment:8 Changed 3 years ago by ncohen

You are the one who makes it personal. I am defending that the path/name of this file does not represent its content, and I explain why. You are welcome to answer the technical comments here. For everything else use emails.

Last edited 3 years ago by ncohen (previous) (diff)

comment:9 in reply to: ↑ 4 Changed 3 years ago by ncohen

the change to index.rst seems not to be correct..

Arggggg... Right!

Actually, I just figured out why the file did not seem to be included in the reference manual already: there is an independent 'finite_rings' folder in the manual, which for some reason is not a subfolder of the 'rings' folder.

http://doc.sagemath.org/html/en/reference/rings/ http://doc.sagemath.org/html/en/reference/finite_rings/

Anyway. That's fixed, and the doc compiles.

Nathann

comment:10 Changed 3 years ago by git

  • Commit changed from 42718e8d42381ba299eae472417402719577795e to 0bc4fc75c99ea6f968e2c72fd71fd33157ed5dd9

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

0bc4fc7trac #19941: Wrong name in index.rst

comment:11 in reply to: ↑ 7 Changed 3 years ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

Replying to davidloeffler:

Nathann: You are out of order, and your comments on this ticket and the associated sage_devel thread fall far below the standard of conduct appropriate for Sage contributors.

David Roe is one of Sage's most valued developers and he has worked tirelessly on Sage's code for finite and p-adic fields.

David, FYI, Nathann just as well has been contributing to Sage quite a bit.

You, on the other hand, clearly know nothing about the content of this file,

David, let me assure you that Nathann certainly perfectly aware of what a finite field is. He has been using them quite a bit...

and thus you are reduced to making aggressive, mocking comments about its name, which is invisible to most Sage users anyway. You may as well write to the authors of a paper in a top journal telling them that you don't like the title they've given their paper.

There is quite a bit of crap in top journals, as we all know, often due to reviewers... Thanks goodness, here we can correct things overlooked by Sage reviewers. This is often thankless job, but it makes code much more readable. And this is very important.

Don't you have anything better to do with your time?

Improving Sage codebase is very important, and we should be thankful to Nathann for this.

comment:12 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict...

comment:13 Changed 3 years ago by git

  • Commit changed from 0bc4fc75c99ea6f968e2c72fd71fd33157ed5dd9 to bd064a69b3574c77263ef98b999d44e84171e9ea

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

bd064a6trac #19941: Merged with 7.1.beta0

comment:14 Changed 3 years ago by ncohen

I just fixed the merge problem. Considering what this ticket does, I think it's best to wait for the patchbot to run the tests again, just to be sure ^^;

comment:15 Changed 3 years ago by git

  • Commit changed from bd064a69b3574c77263ef98b999d44e84171e9ea to 25a961f6e41e97bcebfab2057749b676eb8002c6

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

25a961ftrac #19941: Imports from the latest beta

comment:16 Changed 3 years ago by ncohen

  • Status changed from needs_work to positive_review

It seems to be okay. The broken doctests reported by the patchbot are unrelated.

Nathann

comment:17 Changed 3 years ago by vbraun

  • Branch changed from public/19941 to 25a961f6e41e97bcebfab2057749b676eb8002c6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.