Opened 4 years ago

Last modified 3 months ago

#24934 new defect

Orders are not unique parents

Reported by: saraedum Owned by:
Priority: minor Milestone: sage-9.5
Component: number fields Keywords: padicIMA
Cc: jdemeyer Merged in:
Authors: Julian Rüth Reviewers:
Report Upstream: N/A Work issues: fix maximal orders, remove all calls to AbsoluteOrder/RelativeOrder
Branch: u/saraedum/24934 (Commits, GitHub, GitLab) Commit: f3fa5bea0e14919a5353196d40ebf5b3e40883c4
Dependencies: Stopgaps:

Status badges

Description (last modified by saraedum)

Currently, orders are not unique parents

sage: K.<i> = QuadraticField(-1)
sage: loads(dumps(K.maximal_order())) is K.maximal_order()
False

However, order and maximal_order rely on a @cached_method, so you usually don't see this behaviour:

sage: K.maximal_order() is K.maximal_order()
True

This ticket fixes this by creating orders in a factory.

This ticket also drops the keyword allow_subfield that did not really do anything before, see #16046.

Change History (24)

comment:1 Changed 4 years ago by jdemeyer

  • Cc jdemeyer added

comment:2 Changed 4 years ago by saraedum

  • Description modified (diff)

comment:3 Changed 4 years ago by saraedum

  • Description modified (diff)

comment:4 Changed 4 years ago by saraedum

  • Branch set to u/saraedum/completion

comment:5 Changed 4 years ago by saraedum

  • Commit set to f28c9120a59671d6d2d0478dfa7aec2f0ed8e812
  • Work issues set to fix maximal orders

Maximal orders are not handled correctly by my code:

sage: K.<i> = QuadraticField(-1)
sage: O = K.order(i)
sage: O
Order in Number Field in i with defining polynomial x^2 + 1
sage: K.maximal_order()
Order in Number Field in i with defining polynomial x^2 + 1
sage: O.is_maximal()
True
sage: K.maximal_order()
Gaussian Integers in Number Field in i with defining polynomial x^2 + 1
Last edited 4 years ago by saraedum (previous) (diff)

comment:6 Changed 4 years ago by git

  • Commit changed from f28c9120a59671d6d2d0478dfa7aec2f0ed8e812 to b67108f6249836958df36c4a9e7417776265247c

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

b67108fOrders are unique parents

comment:7 Changed 4 years ago by saraedum

  • Work issues changed from fix maximal orders to fix maximal orders, remove all calls to AbsoluteOrder/RelativeOrder

comment:8 Changed 4 years ago by saraedum

  • Branch u/saraedum/completion deleted
  • Commit b67108f6249836958df36c4a9e7417776265247c deleted

comment:9 Changed 4 years ago by saraedum

  • Branch set to u/saraedum/orders

comment:10 Changed 4 years ago by tscrim

  • Commit set to a39d7b0a908a12563a38a68385afe6caee1a95eb

IMO, you should subclass UniqueRepresentation rather than make a UniqueFactory as it makes the layout easier (no separate factory object or class that handles the uniqueness). While sometimes there are good reasons to not do this, usually I find the code easier to understand and maintain by using UniqueRepresentation.


New commits:

b67108fOrders are unique parents
a39d7b0fix pickling

comment:11 Changed 4 years ago by saraedum

I think that's a matter of taste. I prefer factories.

comment:12 Changed 4 years ago by tscrim

I have always found it harder to do maintenance on UniqueFactory objects than UniqueRepresentation objects due to the increased code complexity and non-locality. Plus UniqueRepresentation has slightly better behavior (see structure/factory.pyx and structure/unique_representation.py docs).

In addition, for UniqueFactory, the documentation either has to be duplicated with the factory and the actual class or you have to pick-and-choose which one gets what documentation. So it makes it hard to work with the interactive documentation.

So in my mind, it is a technical debt taken on to use UniqueFactory when not necessary and not simply a matter of taste.

Last edited 4 years ago by tscrim (previous) (diff)

comment:13 Changed 4 years ago by tscrim

However, I should also clarify that this is only a suggestion, and I would not revert a positive review if you continue to use UniqueFactory.

comment:14 Changed 4 years ago by saraedum

Ok. Thanks for the input. I think I will stick with the factory approach.

comment:15 Changed 4 years ago by git

  • Commit changed from a39d7b0a908a12563a38a68385afe6caee1a95eb to 5c1c320991a5627667d46d3095085734be201348

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

d4d4c35Merge remote-tracking branch 'trac/u/saraedum/orders' into orders
5c1c320add missing doctests

comment:16 Changed 4 years ago by git

  • Commit changed from 5c1c320991a5627667d46d3095085734be201348 to ba5a7d0a16e3f007041f24661070a02aea35303a

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

ba5a7d0Remove unused code

comment:17 Changed 3 years ago by saraedum

  • Branch u/saraedum/orders deleted
  • Commit ba5a7d0a16e3f007041f24661070a02aea35303a deleted

comment:18 Changed 3 years ago by saraedum

  • Branch set to u/saraedum/24934

comment:19 Changed 3 years ago by roed

  • Commit set to f3fa5bea0e14919a5353196d40ebf5b3e40883c4
  • Keywords padicIMA added

New commits:

b67108fOrders are unique parents
a39d7b0fix pickling
d4d4c35Merge remote-tracking branch 'trac/u/saraedum/orders' into orders
5c1c320add missing doctests
ba5a7d0Remove unused code
f3fa5beMerge develop and 24934

comment:20 Changed 3 years ago by chapoton

do not use it.next but next(it) for python3 compatibility, please

comment:21 Changed 15 months ago by mkoeppe

  • Milestone changed from sage-8.2 to sage-9.2

comment:22 Changed 14 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:23 Changed 8 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

comment:24 Changed 3 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5
Note: See TracTickets for help on using tickets.