Opened 4 years ago

Closed 4 years ago

#23761 closed enhancement (fixed)

Add QQ to the category of number fields

Reported by: tscrim Owned by:
Priority: major Milestone: sage-8.1
Component: number fields Keywords:
Cc: chapoton, jdemeyer, delecroix, mderickx Merged in:
Authors: Travis Scrimshaw Reviewers: Maarten Derickx
Report Upstream: N/A Work issues:
Branch: 32a4e16 (Commits, GitHub, GitLab) Commit: 32a4e1655ab6ed74bc90f28b86187fa6918a348b
Dependencies: Stopgaps:

Status badges

Description

Because QQ is a number field, but Sage doesn't recognize it as one:

sage: QQ.categories()
[Join of Category of quotient fields and Category of metric spaces,
 Category of quotient fields,
 Category of fields,
 Category of euclidean domains,
 Category of division rings,
 Category of principal ideal domains,
 Category of unique factorization domains,
 Category of gcd domains,
 Category of integral domains,
 Category of domains,
 Category of commutative rings,
 Category of rings,
 Category of rngs,
 Category of semirings,
 Category of associative additive commutative additive associative additive unital distributive magmas and additive magmas,
 Category of additive commutative additive associative additive unital distributive magmas and additive magmas,
 Category of additive commutative additive associative distributive magmas and additive magmas,
 Category of additive associative distributive magmas and additive magmas,
 Category of distributive magmas and additive magmas,
 Category of magmas and additive magmas,
 Category of commutative monoids,
 Category of monoids,
 Category of semigroups,
 Category of commutative magmas,
 Category of unital magmas,
 Category of magmas,
 Category of commutative additive groups,
 Category of additive groups,
 Category of additive inverse additive unital additive magmas,
 Category of commutative additive monoids,
 Category of additive monoids,
 Category of additive unital additive magmas,
 Category of commutative additive semigroups,
 Category of additive commutative additive magmas,
 Category of additive semigroups,
 Category of additive magmas,
 Category of metric spaces,
 Category of topological spaces,
 Category of sets,
 Category of sets with partial maps,
 Category of objects]

Related to #23245 and #7596.

Change History (10)

comment:1 Changed 4 years ago by tscrim

  • Branch set to public/number_fields/QQ_number_field-23761
  • Commit set to 20d8949ddd671d34067dc7cdd61d24aa580572fa
  • Status changed from new to needs_review

New commits:

20d8949Adding QQ to NumberFields.

comment:2 Changed 4 years ago by git

  • Commit changed from 20d8949ddd671d34067dc7cdd61d24aa580572fa to c028abb2b6f968d5ab37fd22fd895cb7c80de34f

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

c028abbAdding QQ to NumberFields.

comment:3 Changed 4 years ago by git

  • Commit changed from c028abb2b6f968d5ab37fd22fd895cb7c80de34f to 32a4e1655ab6ed74bc90f28b86187fa6918a348b

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

32a4e16Adding QQ to NumberFields.

comment:4 Changed 4 years ago by mderickx

  • Status changed from needs_review to needs_work

Hi Travis, I was just busy reviewing this ticket and then I saw a new commit. I noticed that you like to force push new stuff to trac, but I strongly dislike this. Once you have made your changes public you should not force push, because

1) it screws up other people who have pulled your branch, and made changes on top of it.

2) It destroys history, for example it is now harder to review this ticket for me. If you didn't force push, I could just could have looked at the diff of your most recent commit and your second most recent commit and I could see if the extra additions make sense. Now however I need to start from scratch.

Here is a good read by Linus Torvaldus on how one should use git and be nice to others: http://www.mail-archive.com/dri-devel@lists.sourceforge.net/msg39091.html

Luckily this is a small ticket so I am not to annoyed, but still a little bit annoyed non the less.


New commits:

32a4e16Adding QQ to NumberFields.

comment:5 Changed 4 years ago by mderickx

  • Status changed from needs_work to needs_review

comment:6 Changed 4 years ago by mderickx

  • Status changed from needs_review to positive_review

In my original review I was going to say that it looks good to me except that you missed 4 files where there are also categories printed that depend on the category of QQ. But save that, that it looks good to me.

If you didn't force push you would have saved me the trouble to manually compare if your latest commit did noting except changing those 4 doctests.

P.s. I also tested if this happens to give QQ any new methods, but sadly enough there are no new ones that show up with tab completion. I still think it is a good change though.

comment:7 Changed 4 years ago by tscrim

Thank you for the review. If you could add in your name to the reviewer field.

Thank you also for the lecture. It's not like this ticket was sitting here for that long, so I figured it was safe to do so to keep a clean history. Additionally, from having dealt with merging in rebased branches, git is pretty good with merging equal changes to setup the diff.

comment:8 Changed 4 years ago by tscrim

Sorry, that was very passive-aggressive of me. I didn't think anyone would be looking at this ticket in the timespan from when I first posted it, and I apologize for the trouble.

comment:9 Changed 4 years ago by mderickx

  • Reviewers set to Maarten Derickx

comment:10 Changed 4 years ago by vbraun

  • Branch changed from public/number_fields/QQ_number_field-23761 to 32a4e1655ab6ed74bc90f28b86187fa6918a348b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.