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

Narrow down import of SageObject #30713

Open
tobiasdiez opened this issue Oct 3, 2020 · 15 comments
Open

Narrow down import of SageObject #30713

tobiasdiez opened this issue Oct 3, 2020 · 15 comments

Comments

@tobiasdiez
Copy link
Contributor

Small change of narrowing down the import of SageObject instead of importing the whole module.

CC: @mkoeppe @tscrim

Component: refactoring

Keywords: sd111

Author: Tobias Diez

Branch/Commit: public/refactoring/narrowSageObject @ ff9ed51

Reviewer: Travis Scrimshaw, Matthias Koeppe

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

@tobiasdiez tobiasdiez added this to the sage-9.2 milestone Oct 3, 2020
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 3, 2020

Commit: ff9ed51

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 3, 2020

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

ff9ed51Narrow down import of SageObject

@tobiasdiez
Copy link
Contributor Author

New commits:

ff9ed51Narrow down import of SageObject

New commits:

ff9ed51Narrow down import of SageObject

@mkoeppe
Copy link
Member

mkoeppe commented Oct 3, 2020

comment:3

I also prefer the style that uses from ... import ..., but this is a cosmetic change. It still loads the whole module.

@tscrim
Copy link
Collaborator

tscrim commented Oct 4, 2020

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Oct 4, 2020

comment:4

Replying to @mkoeppe:

I also prefer the style that uses from ... import ...,

Same.

but this is a cosmetic change. It still loads the whole module.

It is not entirely cosmetic with how the imports work IIRC. But it does make is clear what is desired from that module, so I think it is a net improvement.

@mkoeppe
Copy link
Member

mkoeppe commented Oct 4, 2020

Changed reviewer from Travis Scrimshaw to Travis Scrimshaw, Matthias Koeppe

@tobiasdiez
Copy link
Contributor Author

comment:6

Thanks for the review!

@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Oct 24, 2020
@vbraun
Copy link
Member

vbraun commented Oct 25, 2020

comment:8

docbuild fails:

[dochtml] [function_] building [inventory]: targets for 11 source files that are out of date
[dochtml] [function_] updating environment: [new config] 11 added, 0 changed, 0 removed
[dochtml] [combinat ] /home/release/Sage/local/lib/python3.8/site-packages/sage/combinat/crystals/mv_polytopes.py:docstring of sage.combinat.crystals.mv_polytopes.MVPolytope.plot:22: WARNING: Exception occurred in plotting mv_polytopes-1
[dochtml] [combinat ]  from /home/release/Sage/src/doc/en/reference/combinat/sage/combinat/crystals/mv_polytopes.rst:
[dochtml] [combinat ] Traceback (most recent call last):
[dochtml] [combinat ]   File "/home/release/Sage/local/lib/python3.8/site-packages/matplotlib/sphinxext/plot_directive.py", line 472, in run_code
[dochtml] [combinat ]     exec(code, ns)
[dochtml] [combinat ]   File "<string>", line 4, in <module>
[dochtml] [combinat ]   File "/home/release/Sage/local/lib/python3.8/site-packages/sage/combinat/root_system/root_lattice_realizations.py", line 2053, in plot
[dochtml] [combinat ]     G += self.plot_roots(roots, plot_options=plot_options)
[dochtml] [combinat ]   File "/home/release/Sage/local/lib/python3.8/site-packages/sage/combinat/root_system/root_lattice_realizations.py", line 2299, in plot_roots
[dochtml] [combinat ]     roots = Family(roots, self)
[dochtml] [combinat ]   File "/home/release/Sage/local/lib/python3.8/site-packages/sage/sets/family.py", line 406, in Family
[dochtml] [combinat ]     return LazyFamily(indices, function, name)
[dochtml] [combinat ]   File "/home/release/Sage/local/lib/python3.8/site-packages/sage/sets/family.py", line 908, in __init__
[dochtml] [combinat ]     self.set = copy(set)
[dochtml] [combinat ]   File "/home/release/Sage/local/lib/python3.8/copy.py", line 102, in copy
[dochtml] [combinat ]     return _reconstruct(x, None, *rv)
[dochtml] [combinat ]   File "/home/release/Sage/local/lib/python3.8/copy.py", line 264, in _reconstruct
[dochtml] [combinat ]     y = func(*args)
[dochtml] [combinat ]   File "stringsource", line 7, in sage.structure.sage_object.__pyx_unpickle_SageObject (build/cythonized/sage/structure/sage_object.c:11737)
[dochtml] [combinat ] TypeError: sage.structure.sage_object.SageObject.__new__(FiniteFamily_with_category) is not safe, use FiniteFamily_with_category.__new__()
[dochtml] [combinat ] /home/release/Sage/local/lib/python3.8/site-packages/sage/combinat/crystals/mv_polytopes.py:docstring of sage.combinat.crystals.mv_polytopes.MVPolytopes:101: WARNING: Exception occurred in plotting mv_polytopes-2

@tobiasdiez
Copy link
Contributor Author

comment:9

Thanks for testing, but I don't see how this error TypeError: sage.structure.sage_object.SageObject.__new__(FiniteFamily_with_category) is not safe, use FiniteFamily_with_category.__new__() is related to the changes in this branch. Do you have any suggestions on how to investigate this?

@tscrim
Copy link
Collaborator

tscrim commented Oct 27, 2020

comment:10

Something very subtle is likely happening if it is indeed related to this branch. The first thing to do would be to try and reproduce the error. You might want to try with Volker's current branch to see if it is coming from one of the already merged-but-not-released branches for the upcoming 9.3.beta0. However, I am at a loss as well for the error and reproducing it.

@mkoeppe
Copy link
Member

mkoeppe commented Dec 6, 2020

comment:11

Hoping we can make progress on this ticket this week - https://wiki.sagemath.org/days111

@mkoeppe
Copy link
Member

mkoeppe commented Dec 6, 2020

Changed keywords from none to sd111

@mkoeppe
Copy link
Member

mkoeppe commented Feb 13, 2021

comment:12

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

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Feb 13, 2021
@mkoeppe
Copy link
Member

mkoeppe commented Jul 19, 2021

comment:13

Setting a new milestone for this ticket based on a cursory review.

@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Jul 19, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 18, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Apr 2, 2022
@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Aug 31, 2022
@mkoeppe mkoeppe removed this from the sage-9.8 milestone Jan 29, 2023
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

4 participants