sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.44k stars 481 forks source link

Remove Python access from Parent._element_constructor #23881

Closed jdemeyer closed 7 years ago

jdemeyer commented 7 years ago

In order to avoid confusion between _element_constructor_ (a public interface) and _element_constructor (a private attribute), we change the latter from cdef public to cdef. The same for _element_init_pass_parent. This way, those attributes can no longer be accessed from Python.

Depends on #23882 Depends on #23884 Depends on #23894 Depends on #23899 Depends on #23905 Depends on #23907 Depends on #23914 Depends on #24033

CC: @simon-king-jena @koffie

Component: coercion

Author: Jeroen Demeyer, Simon King

Branch: 41614a7

Reviewer: Travis Scrimshaw

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

jdemeyer commented 7 years ago

Changed branch from u/SimonKing/rename_parentelementconstructor____parentconstruct_element to u/jdemeyer/rename_parentelementconstructor____parentconstruct_element

jdemeyer commented 7 years ago

Changed commit from c95b724 to 41614a7

jdemeyer commented 7 years ago

New commits:

6878e67Avoid _element_constructor in padics
44bc9fbDo not assign _element_constructor in combinat/sf
f684cb0Implement `_element_constructor_` for Homset
8b9765eAvoid _element_constructor in geometry doctests
2630849Various minor changes to _element_constructor
2856051Merge commit '6878e67e99f1f3707c8b4a060264f5484989c52b'; commit '44bc9fb660fe4508b0bd8340667b8b65b70226a1'; commit 'f684cb0e4834a0041da29ef7a1f8f0b01cf84994'; commit '8b9765e781f8f9359cfc6021f6cd3d0e3b4b36d6'; commit '2630849a988376b44769460989a41919bbe89520' into t/23881/rename_parent__element_constructor____parent___construct_element
77ccb85Do not set _element_init_pass_parent
41614a7Remove Python access from Parent._element_constructor and Parent._element_init_pass_parent
tscrim commented 7 years ago
comment:51

Green patchbot => positive review.

tscrim commented 7 years ago

Reviewer: Travis Scrimshaw

koffie commented 7 years ago
comment:52

Both arando and rk02-math seem to fail the same test:

 sage -t --long src/sage/schemes/elliptic_curves/sha_tate.py # 1 doctest failed 

Which they do not seem to fail on with other tickets.

tscrim commented 7 years ago
comment:53

I don't see how those failures could be related to the changes in this ticket unless they appear also in one of the dependencies. Even still, I don't see how they could be connected, much less be machine dependent.

koffie commented 7 years ago
comment:54

I don't see see why either so I agree it is highly unlikely, but since parent is modified it is not impossible. It does not need to be machine dependent, it can also be a bug that happens 50% of the time that is exposed after this.

There are two machines failing on this ticket and I have not seen it on either arando or rk02-math in at least 10 other failing logs that I checked. So from this point of view it is also very unlikely that it is not from this ticket. Of course this ticket could have been very unlucky to expose a rare random bug twice for the first time but this is also highly unlikely.

I just would like to see arguments for why we are in one unlikely scenario and not the other.

Note that this ticket also has a lot of dependencies merged so it could also be exposed by one of the dependencies, which might explain why I didn't see it on any of the arando and rk02-math logsI checked.

If only we could search through the patchbot log easily, then we could get conclusive answers much faster.

jdemeyer commented 7 years ago
comment:55

I don't think that the failures have anything to do with this ticket, so I'm setting it to positive_review. If the failures are real, then it will fail on Volker's buildbot anyway.

jdemeyer commented 7 years ago
comment:56

I also tested it on arando manually (without the patchbot) and that test passed.

koffie commented 7 years ago
comment:57

Before you reported that arando passes all test I kicked the patchbot to get more info. And arando did fail again on the run resulting from that kick, while the bug did not occur in two tests ran on arando in between.

I am a bit mystified by the situation since it can't be the optional packages since rk02-math has none, so it must be a randomly occuring bug.

koffie commented 7 years ago
comment:58

I tested this with

./sage -t --long --global-iterations=100 src/sage/schemes/elliptic_curves/sha_tate.py

on arando and they all pass so it is not easily reproducible.

koffie commented 7 years ago
comment:59

The sha_tate doctest fails on sage8.1.beta7 without any patches on arando so I created #23962, since arando failed on the 3 tests it ran on this ticket, but none of the other beta6 tests, I still suspect it to be something introduced by one of the dependencies of this ticket that got merged in sage8.1.beta7. Anyway it can't be this ticket so lets move the discussion to #23962.

jdemeyer commented 7 years ago
comment:60

Replying to @tscrim:

to prevent anyone else from adding something (although I would say that is highly unlikely).

It seems that precisely this has happened in #23660...

jdemeyer commented 7 years ago

Changed dependencies from #23882, #23884, #23894, #23899, #23905, #23907, #23914 to #23882, #23884, #23894, #23899, #23905, #23907, #23914, #24033

vbraun commented 7 years ago

Changed branch from u/jdemeyer/rename_parentelementconstructor____parentconstruct_element to 41614a7

jdemeyer commented 7 years ago
comment:63

Good that this is merged now...

jdemeyer commented 7 years ago

Changed commit from 41614a7 to none