On Sunday, 26 September 2021 at 10:44:14 UTC-7 Matthias Koeppe wrote: > *1. Replace use of "from sage.PAC.KAGE.all import ..." by more specific > imports* >
I'm not sure whether I'm a fan of the "all" packages (for one thing, they make proper use of "lazy_import" a lot harder, but importing lazy_import from all will cause exactly the same problem), but does it need to stand in the way of modularization? Doesn't it just mean that the "all" package needs to do a little magic to discover what its namespace should contain? Here I mean with "magic" something that perhaps needs to discover during runtime what is available within the relevant "namespace" package. One big advantage of "all" packages is that you can hide the internal module structure of a package with it a little bit, so that the structure becomes an implementation detail, not a part of the API specification. I think that has served us well in the past on some occasions. I'm not sure we want to surrender that option if we don't have to. > *2. Do not import CLASS just to run an isinstance(..., CLASS); likewise, > remove uses of is_CLASS functions* > > Instead, create abstract base classes that can be imported without pulling > in the actual implementation classes ( > https://trac.sagemath.org/ticket/32414). I have done this in > https://trac.sagemath.org/ticket/32566 for real/complex floating point > fields. > Have you considered performance implications of that? It would look to me that abstract base classes require more work to determine membership than a plain MRO lookup. I think this "rule" probably needs a little more cautious formulation. I suspect there may be good reasons to do specific isinstance(...,CLASS) tests. Perhaps this is captured in your "...just to run...". I'm not sure about performance issues compared to "is_CLASS" functions, but clearly banning them should be accompanied by an assessment of why their replacement by "isinstance(...)" is not a (performance) regression. *3. Break up Cython modules that depend on several C/C++ libraries > simultaneously* > > An example is sage.matrix.misc, which depends on both flint and mpfr. This > is https://trac.sagemath.org/ticket/32433 (which needs help). > The particular file you point to looks like it deserves to be restructured, but the way your objective is structured seems definitely flawed. I just wrote a cython module that essentially depends on both the mpfr and the pari libraries (on C-level!). I don't see how that would be broken up and why it would even be bad to link a cython module against several libraries at once. -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/sage-devel/9d11a766-3d8e-4768-ad93-9d2cafef777en%40googlegroups.com.
