Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add _make_named_class_key for several categories, miscellaneous changes #39160

Merged
merged 5 commits into from
Feb 10, 2025

Conversation

user202729
Copy link
Contributor

@user202729 user202729 commented Dec 18, 2024

Fixes #39154

Let C = Category.join([NumberFields(), QuotientFields(), MetricSpaces()]), then there are 4 categories being created

  • Modules(C)
  • VectorSpaces(C)
  • Modules(C).Filtered()
  • VectorSpaces(C).Filtered()

The last 2 have type FilteredModulesCategory, which inherit from RegressiveCovariantConstructionCategory, Category_over_base_ring which in turn inherits from CategoryWithParameters. Subclasses of CategoryWithParameters need to override _make_named_class_key correctly so that category A and B can only return same key if their parent class, element class, subcategory class etc. are identical. But FilteredModulesCategory takes _make_named_class_key from Category_over_base_ring, which returns the base category (C) for the key.

With this change, the returned key is e.g. Modules(C).parent_class and VectorSpaces(C).parent_class respectively, which is different.

Side note: by how things are implemented in SageMath, Modules(C) actually returns the same thing as VectorSpaces(C). The above diagram is only for illustration purpose.

Side note 2: Modules(C).Filtered() is currently a subcategory of VectorSpaces(C)… so the above is not a diamond, rather it's a linear chain.

Also make Modules(Fields())).Filtered()VectorSpaces(Fields()), and a few other changes e.g. make _test_category_graph works for hom categories.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Copy link

github-actions bot commented Dec 18, 2024

Documentation preview for this PR (built with commit a7e12d6; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@user202729 user202729 marked this pull request as draft December 19, 2024 02:23
@user202729 user202729 force-pushed the functorial-category-cache-key branch 2 times, most recently from 06d6368 to c6b3b55 Compare December 19, 2024 16:23
@user202729 user202729 marked this pull request as ready for review December 19, 2024 23:37
Copy link
Contributor

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! I can confirm that the linked bug is indeed fixed by these changes. Tests pass as well (modulo the usual random issues).

I don't really feel comfortable reviewing such fundamental changes in the category framework. So it would be awesome if one of the experts have a look as well.

@user202729
Copy link
Contributor Author

user202729 commented Dec 20, 2024

Actually while working on this I realize the bug runs deeper than it looked. Just see how many tests #39170 breaks.

The current implementation is suboptimal as well (but yes, at least it works slightly better than the previous one).

I think @simon-king-jena is the best one tocomment on this (as author of #11943 and #11935 ) butthe PR was 13 years ago so I've no idea if the author is still contactable.


There's a better way to implement this. Will copy from #39170 once it works.

@user202729 user202729 changed the title Add _make_named_class_key for FunctorialConstructionCategory Add _make_named_class_key for FilteredModules Dec 20, 2024
@user202729 user202729 marked this pull request as draft December 21, 2024 03:16
@tobiasdiez tobiasdiez requested a review from tscrim December 22, 2024 07:23
@user202729 user202729 force-pushed the functorial-category-cache-key branch from b5176a5 to 97db4bf Compare December 23, 2024 09:14
@user202729 user202729 marked this pull request as ready for review December 23, 2024 09:14
@user202729
Copy link
Contributor Author

user202729 commented Dec 23, 2024

I implement the cleaner solution (just getattr(category, base)), as well as modifying the documentation to add what I found out during the hours of debugging. Hopefully it saves whoever comes across this function some time.

The debug option to check the MRO is not in though (#39170). That would take more time.

@user202729 user202729 changed the title Add _make_named_class_key for FilteredModules Add _make_named_class_key for several categories, miscellaneous changes Dec 24, 2024
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 9, 2025
sagemathgh-39160: Add _make_named_class_key for several categories, miscellaneous changes
    
Fixes sagemath#39154

Let `C = Category.join([NumberFields(), QuotientFields(),
MetricSpaces()])`, then there are 4 categories being created

* `Modules(C)`
* `VectorSpaces(C)`
* `Modules(C).Filtered()`
* `VectorSpaces(C).Filtered()`

The last 2 have type `FilteredModulesCategory`, which inherit from
`RegressiveCovariantConstructionCategory, Category_over_base_ring` which
in turn inherits from `CategoryWithParameters`. Subclasses of
`CategoryWithParameters` need to override `_make_named_class_key`
correctly so that category A and B can only return same key if their
parent class, element class, subcategory class etc. are identical. But
`FilteredModulesCategory` takes `_make_named_class_key` from
`Category_over_base_ring`, which returns the base category (`C`) for the
key.

With this change, the returned key is e.g. `Modules(C).parent_class` and
`VectorSpaces(C).parent_class` respectively, which is different.

Side note: by how things are implemented in SageMath, `Modules(C)`
actually returns the same thing as `VectorSpaces(C)`. The above diagram
is only for illustration purpose.

Side note 2: `Modules(C).Filtered()` is currently a subcategory of
`VectorSpaces(C)`… so the above is not a diamond, rather it's a linear
chain.

Also make `Modules(Fields())).Filtered()` ≤ `VectorSpaces(Fields())`,
and a few other changes e.g. make `_test_category_graph` works for hom
categories.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39160
Reported by: user202729
Reviewer(s): Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 9, 2025
sagemathgh-39160: Add _make_named_class_key for several categories, miscellaneous changes
    
Fixes sagemath#39154

Let `C = Category.join([NumberFields(), QuotientFields(),
MetricSpaces()])`, then there are 4 categories being created

* `Modules(C)`
* `VectorSpaces(C)`
* `Modules(C).Filtered()`
* `VectorSpaces(C).Filtered()`

The last 2 have type `FilteredModulesCategory`, which inherit from
`RegressiveCovariantConstructionCategory, Category_over_base_ring` which
in turn inherits from `CategoryWithParameters`. Subclasses of
`CategoryWithParameters` need to override `_make_named_class_key`
correctly so that category A and B can only return same key if their
parent class, element class, subcategory class etc. are identical. But
`FilteredModulesCategory` takes `_make_named_class_key` from
`Category_over_base_ring`, which returns the base category (`C`) for the
key.

With this change, the returned key is e.g. `Modules(C).parent_class` and
`VectorSpaces(C).parent_class` respectively, which is different.

Side note: by how things are implemented in SageMath, `Modules(C)`
actually returns the same thing as `VectorSpaces(C)`. The above diagram
is only for illustration purpose.

Side note 2: `Modules(C).Filtered()` is currently a subcategory of
`VectorSpaces(C)`… so the above is not a diamond, rather it's a linear
chain.

Also make `Modules(Fields())).Filtered()` ≤ `VectorSpaces(Fields())`,
and a few other changes e.g. make `_test_category_graph` works for hom
categories.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39160
Reported by: user202729
Reviewer(s): Tobias Diez
@vbraun vbraun merged commit fc97dcd into sagemath:develop Feb 10, 2025
23 checks passed
@tscrim
Copy link
Collaborator

tscrim commented Feb 11, 2025

Sorry for not checking this sooner. I am not sure about the changes to how the categories are construction. @nthiery @hivert Perhaps you have a comment about that?

That being said, HomsetsCategory._make_named_class_key is a method added that does not have a doctest. Please add one in a followup PR (as that almost always a requirement for merging code).

vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 18, 2025
sagemathgh-39509: Add a doctest for HomsetsCategory._make_named_class_key
    
Follow-up to sagemath#39160 .



### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview. (no visible change, method is private.)

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39509
Reported by: user202729
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 21, 2025
sagemathgh-39509: Add a doctest for HomsetsCategory._make_named_class_key
    
Follow-up to sagemath#39160 .



### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview. (no visible change, method is private.)

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39509
Reported by: user202729
Reviewer(s):
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot create consistent method resolution order (MRO) when using filtered categories
4 participants