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

Move attrcall and friends from sage.misc.misc to new module sage.misc.call #29869

Closed
mkoeppe opened this issue Jun 15, 2020 · 25 comments
Closed

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Jun 15, 2020

This will help with #29865

CC: @tscrim

Component: refactoring

Author: Matthias Koeppe

Branch/Commit: 6024ffd

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/29869

@mkoeppe mkoeppe added this to the sage-9.2 milestone Jun 15, 2020
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 15, 2020

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 15, 2020

Commit: 907feeb

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 15, 2020

comment:3

The reimport in sage.misc.misc probably needs a deprecation?


New commits:

907feebMove attrcall and friends from sage.misc.misc to new module sage.misc.call

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 15, 2020

Author: Matthias Koeppe

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2020

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

a5453bfFixup: Add src/sage/misc/call.py

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2020

Changed commit from 907feeb to a5453bf

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2020

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

64c5701lazy_import from sage.misc.call with deprecation
65414f7Fix imports and one deprecation warning

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2020

Changed commit from a5453bf to 65414f7

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 15, 2020

comment:7

I think this is clean now, needs review

@tscrim
Copy link
Collaborator

tscrim commented Jun 15, 2020

comment:8

You need to add the new file to

src/doc/en/reference/misc/index.rst

It is also missing the standard header information:

"""
Miscellaneous calling functions
"""

# Copyright...

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2020

Changed commit from 65414f7 to b9314d4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2020

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

b9314d4sage.misc.call: Add standard header information, add to reference manual

@tscrim
Copy link
Collaborator

tscrim commented Jun 16, 2020

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Jun 16, 2020

comment:10

Thanks. If the patchbot comes back green, then this is a positive review.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 16, 2020

comment:11

Well, there are all kinds of colors, but I think this is from code that was already there

@tscrim
Copy link
Collaborator

tscrim commented Jun 16, 2020

comment:12

There is one real failure:

sage -t --long src/sage/modules/with_basis/indexed_element.pyx  # 1 doctest failed

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 17, 2020

comment:13
explain_pickle(x)
pg_dynamic_class = unpickle_global('sage.structure.dynamic_class', 'dynamic_class')
pg_CombinatorialFreeModuleElement = unpickle_global('sage.combinat.free_module', 'CombinatorialFreeModuleElement')
pg_getattr = unpickle_global('__builtin__', 'getattr')
pg_call_method = unpickle_global('sage.misc.misc', 'call_method')
pg_unreduce = unpickle_global('sage.structure.unique_representation', 'unreduce')
pg_Modules = unpickle_global('sage.categories.modules', 'Modules')
pg_IntegerRing = unpickle_global('sage.rings.integer_ring', 'IntegerRing')
pg = unpickle_instantiate(pg_IntegerRing, ())
si1 = pg_call_method(pg_unreduce(pg_Modules, (pg,), {'dispatch':False}), '_with_axiom', 'WithBasis')
si2 = unpickle_newobj(pg_dynamic_class('CombinatorialFreeModule_with_category.element_class', (pg_CombinatorialFreeModuleElement, pg_getattr(pg_call_method(si1, '_with_axiom', 'FiniteDimensional'), 'element_class')), None, None, None), ())
pg_CombinatorialFreeModule = unpickle_global('sage.combinat.free_module', 'CombinatorialFreeModule')
pg_FiniteEnumeratedSet = unpickle_global('sage.sets.finite_enumerated_set', 'FiniteEnumeratedSet')
pg_make_integer = unpickle_global('sage.rings.integer', 'make_integer')
unpickle_build(si2, (pg_unreduce(pg_CombinatorialFreeModule, (pg, pg_unreduce(pg_FiniteEnumeratedSet, (('x', 'y'),), {})), {'category':si1, 'prefix':'B'}), {'_monomial_coefficients':{'y':pg_make_integer('2'), 'x':pg_make_integer('2')}}))
si2

OK... I need some help here. Do I need to add register_unpickle_override?

@tscrim
Copy link
Collaborator

tscrim commented Jun 17, 2020

comment:14

Yes, that is correct. You can add it in the old file to redirect to the new file. The other option (which has happened) is to simply drop the backwards compatibility and tell people to find a version of Sage during the deprecation period to load it then resave as a new pickle. IMO, I would rather do the former here since it is a 1-line fix.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 17, 2020

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

6024ffdsrc/sage/misc/call.py: register_unpickle_override for call_method

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 17, 2020

Changed commit from b9314d4 to 6024ffd

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 17, 2020

comment:16

Thanks. I had to add it to the new file though because sage.misc.misc is a dependency of sage.misc.persist.


New commits:

6024ffdsrc/sage/misc/call.py: register_unpickle_override for call_method

@tscrim
Copy link
Collaborator

tscrim commented Jun 17, 2020

comment:17

I think it doesn't matter where it is at. I just want to see one more run of the patchbot before setting a positive review.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 17, 2020

comment:19

Thanks!

@vbraun
Copy link
Member

vbraun commented Jul 2, 2020

@vbraun vbraun closed this as completed in 0c5a199 Jul 2, 2020
mkoeppe added a commit to mkoeppe/sage that referenced this issue Apr 25, 2024
mkoeppe added a commit to mkoeppe/sage that referenced this issue Apr 27, 2024
vbraun pushed a commit to vbraun/sage that referenced this issue May 2, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Remove code deprecated in
- sagemath#32987 (2022)
- sagemath#33213 (2022)
- sagemath#29869 (2020)
- sagemath#17815 (2020)
- sagemath#29892 (2020)

### 📝 Checklist

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

- [x] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [x] 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

<!-- 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#37868
Reported by: Matthias Köppe
Reviewer(s): Michael Orlitzky
@mantepse mantepse mentioned this issue May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants