Opened 4 months ago

Closed 3 months ago

#32112 closed defect (fixed)

Bug in Chart.__init__ regarding the determination of top charts

Reported by: egourgoulhon Owned by:
Priority: major Milestone: sage-9.4
Component: manifolds Keywords: coordinate chart
Cc: mkoeppe, tscrim, gh-mjungmath, vbraun Merged in:
Authors: Eric Gourgoulhon Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: public/manifolds/top_charts_32112 (Commits, GitHub, GitLab) Commit: 4e316e9f4e3e714192568594197afb34cfd8121f
Dependencies: Stopgaps:

Status badges

Description

As noticed in ​https://trac.sagemath.org/ticket/31901#comment:28, there is an issue when constructing two charts sharing the same coordinate symbols but not being otherwise related: 

sage: M = Manifold(2, 'M', structure='topological')
sage: U = M.open_subset('U')
sage: V = M.open_subset('V')
sage: XU = U.chart('x y')
sage: XV = V.chart('x y')
sage: M.top_charts()
[Chart (U, (x, y))]

The chart XV should also appear as a top chart on M.

Change History (8)

comment:1 Changed 4 months ago by egourgoulhon

  • Authors set to Eric Gourgoulhon
  • Branch set to public/manifolds/top_charts_32112
  • Cc mkoeppe tscrim gh-mjungmath added
  • Commit set to 4e316e9f4e3e714192568594197afb34cfd8121f
  • Status changed from new to needs_review

New commits:

​4e316e9Fix bug in Chart.__init__ regarding the determination of top charts (Trac #32112)

comment:2 Changed 4 months ago by mkoeppe

  • Reviewers set to Matthias Koeppe
  • Status changed from needs_review to positive_review

Thanks! This is working well.

comment:3 follow-up: Changed 4 months ago by mkoeppe

It would have been good to do this on top of #32009 though

comment:4 in reply to: ↑ 3 Changed 4 months ago by egourgoulhon

Thanks for the review.

Replying to mkoeppe:

It would have been good to do this on top of #32009 though

Could you explain why? I mean, why _domain should become a Cython attribute?

comment:5 follow-up: Changed 4 months ago by mkoeppe

In #31894 I hope to make Chart a subclass of sage.categories.map.Map so that all functionalities of morphisms such as composition etc. become available. Map is a Cython class in which _domain happens to be a Cython attribute.

comment:6 Changed 4 months ago by mkoeppe

As #32009 came back with a bug, I have made the present ticket a dependency of #32009 instead.

comment:7 in reply to: ↑ 5 Changed 4 months ago by egourgoulhon

Replying to mkoeppe:

In #31894 I hope to make Chart a subclass of sage.categories.map.Map so that all functionalities of morphisms such as composition etc. become available. Map is a Cython class in which _domain happens to be a Cython attribute.

Thanks for the explanation.

comment:8 Changed 3 months ago by mkoeppe

  • Cc vbraun added
  • Resolution set to fixed
  • Status changed from positive_review to closed

Apparently this has been merged as part of #32089.

Note: See TracTickets for help on using tickets.