Opened 5 years ago
Closed 5 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 5 years ago by
- Branch set to public/19941
- Commit set to 944735b847af910a17e6e4dafb771ef83382cf54
- Status changed from new to needs_review
comment:2 Changed 5 years ago by
- Commit changed from 944735b847af910a17e6e4dafb771ef83382cf54 to 7e1852a6e5d477c14a62631128e83618e860d9a6
comment:3 Changed 5 years ago by
- Commit changed from 7e1852a6e5d477c14a62631128e83618e860d9a6 to 42718e8d42381ba299eae472417402719577795e
comment:4 follow-up: ↓ 9 Changed 5 years ago by
the change to index.rst seems not to be correct..
comment:5 follow-up: ↓ 6 Changed 5 years ago by
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 5 years ago by
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: ↓ 11 Changed 5 years ago by
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 5 years ago by
You are the one who makes it personal. I am defending that the title 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.
comment:9 in reply to: ↑ 4 Changed 5 years ago by
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 5 years ago by
- Commit changed from 42718e8d42381ba299eae472417402719577795e to 0bc4fc75c99ea6f968e2c72fd71fd33157ed5dd9
Branch pushed to git repo; I updated commit sha1. New commits:
0bc4fc7 | trac #19941: Wrong name in index.rst
|
comment:11 in reply to: ↑ 7 Changed 5 years ago by
- 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 5 years ago by
- Status changed from positive_review to needs_work
Merge conflict...
comment:13 Changed 5 years ago by
- Commit changed from 0bc4fc75c99ea6f968e2c72fd71fd33157ed5dd9 to bd064a69b3574c77263ef98b999d44e84171e9ea
Branch pushed to git repo; I updated commit sha1. New commits:
bd064a6 | trac #19941: Merged with 7.1.beta0
|
comment:14 Changed 5 years ago by
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 5 years ago by
- Commit changed from bd064a69b3574c77263ef98b999d44e84171e9ea to 25a961f6e41e97bcebfab2057749b676eb8002c6
Branch pushed to git repo; I updated commit sha1. New commits:
25a961f | trac #19941: Imports from the latest beta
|
comment:16 Changed 5 years ago by
- 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 5 years ago by
- Branch changed from public/19941 to 25a961f6e41e97bcebfab2057749b676eb8002c6
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
trac #19941: Rename rings.finite_rings.constructor to finite_field_constructor