Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#23184 closed enhancement (fixed)

Use the right category for DefaultConvertMaps, rather than SetsWithPartialMaps

Reported by: roed Owned by:
Priority: major Milestone: sage-8.0
Component: coercion Keywords: sd86.5
Cc: Merged in:
Authors: David Roe, Julian Rüth Reviewers: Julian Rüth, David Roe
Report Upstream: N/A Work issues:
Branch: 5eccb52 (Commits, GitHub, GitLab) Commit:
Dependencies: #23201 Stopgaps:

Status badges

Description

Currently, the category for DefaultConvertMaps is SetsWithPartialMaps().

sage: type(QQ[['x']].coerce_map_from(QQ))
<type 'sage.structure.coerce_maps.DefaultConvertMap_unique'>
sage: QQ[['x']].coerce_map_from(QQ).category_for()
Category of sets with partial maps

In contrast,

sage: QQ.hom(QQ[['x']]).category_for()
Category of euclidean domains

Change History (17)

comment:1 Changed 4 years ago by roed

  • Dependencies set to #23201
  • Keywords sd86.5 added

comment:2 follow-up: Changed 4 years ago by pbruin

Please make sure that you only refine the category when this makes mathematical sense. In #15618, I implemented taking SetsWithPartialMaps as the default category for a DefaultConvertMap because in general such a map is not defined everywhere, nor is it always a morphism in the category where the parents live.

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

I guess this should be fixed in PowerSeries_generic._coerce_map_from_() by returning a specific map instead of True.

comment:4 in reply to: ↑ 2 Changed 4 years ago by roed

Replying to pbruin:

Please make sure that you only refine the category when this makes mathematical sense. In #15618, I implemented taking SetsWithPartialMaps as the default category for a DefaultConvertMap because in general such a map is not defined everywhere, nor is it always a morphism in the category where the parents live.

My plan is to only change the category for coercions, not conversions. I can't think of any examples where coercions aren't morphisms in the meet of the categories of the parents involved. All of the examples I see in #15618 are conversions, which this ticket won't affect.

comment:5 in reply to: ↑ 3 Changed 4 years ago by roed

Replying to pbruin:

I guess this should be fixed in PowerSeries_generic._coerce_map_from_() by returning a specific map instead of True.

You could fix it in this way, but that's a lot of work that would need to be duplicated over and over again for different examples of DefaultConvertMaps.

comment:6 Changed 4 years ago by roed

  • Branch set to u/roed/use_the_right_category_for_defaultconvertmaps__rather_than_setswithpartialmaps

comment:7 Changed 4 years ago by roed

  • Authors set to David Roe
  • Commit set to 3cc08d1ad81c264d7af4794ed9e41387d645edc2
  • Status changed from new to needs_review

New commits:

b00d97fMake DefaultConvertMaps have a more refined category (than SetsWithPartialMaps) when they're actually coercions
5fbc34cChange base ring for CuspFormsRing in Hecke triangle group
906d5b2Fix base rings for many other parents in sage/modular/moform_hecketriangle
3e0fcd7Merge branch 't/23201/hecke_triangle_group_cusp_form_base_ring' into t/23184/use_the_right_category_for_defaultconvertmaps__rather_than_setswithpartialmaps
3cc08d1Fix a problem in quivers from changing the category of DefaultConvertMaps, add doctests to _generic_convert_map

comment:8 Changed 4 years ago by roed

All tests pass.

comment:9 Changed 4 years ago by git

  • Commit changed from 3cc08d1ad81c264d7af4794ed9e41387d645edc2 to 09d34563e2dd6359b2350e8a4865b45b9a49b8b3

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

09d3456Fix a dangling sage: prompt

comment:10 Changed 4 years ago by saraedum

Looks good except for a few places where S.category() should be replaced with an appropriate meet. I'll fix that myself.

comment:11 Changed 4 years ago by saraedum

  • Branch changed from u/roed/use_the_right_category_for_defaultconvertmaps__rather_than_setswithpartialmaps to u/saraedum/use_the_right_category_for_defaultconvertmaps__rather_than_setswithpartialmaps

comment:12 Changed 4 years ago by git

  • Commit changed from 09d34563e2dd6359b2350e8a4865b45b9a49b8b3 to 5eccb5217999af4dc094b8a21d8b00071457c04d

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

5eccb52Refactor _default_convert_map

comment:13 Changed 4 years ago by saraedum

  • Authors changed from David Roe to David Roe, Julian Rüth
  • Reviewers set to Julian Rüth

comment:14 Changed 4 years ago by saraedum

tests pass.

comment:15 Changed 4 years ago by roed

  • Reviewers changed from Julian Rüth to Julian Rüth, David Roe
  • Status changed from needs_review to positive_review

Looks good.

comment:16 Changed 4 years ago by vbraun

  • Branch changed from u/saraedum/use_the_right_category_for_defaultconvertmaps__rather_than_setswithpartialmaps to 5eccb5217999af4dc094b8a21d8b00071457c04d
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:17 Changed 4 years ago by chapoton

  • Commit 5eccb5217999af4dc094b8a21d8b00071457c04d deleted

you used a bad syntax for the trac role here :

+        We check that `trac`:23184 has been resolved::

it should have been

+        We check that :trac:`23184` has been resolved::

So please review #23526

Last edited 4 years ago by chapoton (previous) (diff)
Note: See TracTickets for help on using tickets.