search

oscar-system / Oscar.jl

A comprehensive open source computer algebra system for computations in algebra, geometry, and number theory.
https://www.oscar-system.org
Other
345 stars 126 forks source link

Do we need Julia (sub)modules for code organization in OSCAR? #2052

Closed HereAround closed 1 year ago

HereAround commented 1 year ago

I recently investigate the OSCAR code and found that we have various Julia modules. Among others the following exist:

Various questions come to my mind:

It seems that @lkastner and I share the opinion that the (sub)modules are not strictly needed. But maybe we are overlooking something fundamental? In extending - maybe OSCAR could benefit from having all (sub)modules removed? Opinions?

cc @fingolfin @fieker @decker @micjoswig

micjoswig commented 1 year ago

I tend to think that submodules are not necessary. But I do not have a strong opinion here.

fieker commented 1 year ago

On Wed, Mar 15, 2023 at 04:14:38AM -0700, Michael Joswig wrote:

I tend to think that submodules are not necessary. But I do not have a strong opinion here.

I'd say yes - to introduce local name spaces. submodules allow me to write and use helper functions that do not interfere with the more global name space - I'd use it as I would use "static" functions in a c-file -- Reply to this email directly or view it on GitHub: https://github.com/oscar-system/Oscar.jl/issues/2052#issuecomment-1469815550 You are receiving this because you were mentioned.

Message ID: @.***>

benlorenz commented 1 year ago

I would prefer keeping some sub-modules. Name-clashes and hiding internal stuff is one reason.

I also think that keeping different submodules will help writing cleaner code. Different subprojects should use the exported APIs of other submodules and not mess too much with their internals and I think submodules do help for this. It makes the code easier to maintain and improves (+ tests) the APIs (as they are in active use that way).

fingolfin commented 1 year ago
  • Do we really need (sub)modules in OSCAR?

Modules are a very useful tool in some cases, I don't see a good reason to forbid them, but many good reasons to allow them. Sure, they "are not strictly needed", but: I also I wouldn't want to forbid for loops, despite the fact that they can all be rewritten as while loops and thus also "are not strictly needed" :-).

You talk about import but reference an issue talking about export. Which is it? I'll assume you mean both for now.

As I see it, The spirit of the rules for import and export are what matters, not their exact current wording. We are free to reword them, too. The intention of those rules is to make it easier to figure out what we import / export (and from where), and to also reduce the mental burden of choosing were to put an import/export statement.

I don't see how submodules prevent this? Sure, they prevent us from putting all import/export statements into a single file each, but that's not a primary objective to me, just something we do because it supports the primary objective (make it easy to determine what we import/export, and where)

  • Do the additional import/using statements in the (sub)modules slow down OSCAR's loading (<-> invalidations)?

No. Invalidations come from other packages and/or the Julia base library.

  • Does the same apply to the experimental tests, as these also contain additional import statements?

I am not sure I understand correctly what you are referring to. I assume the use of import inside of test, i.e.:

test/Experimental/JuLie/schur_polynomials.jl:1:import Oscar.JuLie: schur_polynomial_cbf
test/Experimental/galois-test.jl:35:import Oscar.GaloisGrp: primitive_by_shape, an_sn_by_shape, cycle_structures
test/Groups/conformance.jl:3:import Oscar.AbstractAlgebra.GroupsCore
test/Rings/integer-localizations.jl:8:import Oscar: base_ring, inverted_set, ambient_ring, Localization, parent, numerator, denominator, one, zero, reduce_fraction
test/Rings/integer-localizations.jl:9:import Oscar.AbstractAlgebra: elem_type, parent_type
test/Rings/nmod-localizations.jl:8:import Oscar.Nemo.zzModRing
test/Rings/nmod-localizations.jl:9:import Oscar: base_ring, inverted_set, ambient_ring, Localization, parent, numerator, denominator, one, zero
test/Rings/nmod-localizations.jl:10:import Oscar.AbstractAlgebra: elem_type, parent_type

The majority (or all?) of these outside test/Experimental can and should be removed, I think (in some cases this may require further adjustments to the tests, e.g. for zzModRing). Same for using statements.

For those in test/Experimental: dunno. If the plan is to export them on the long run, they might make sense? In the end, what's the harm, though?

fingolfin commented 1 year ago

Huh, test/Rings/nmod-localizations.jl is really weird. It contains exports, docstrings, and defines a bunch of code. I'll open an issue

fingolfin commented 1 year ago

We discussed this today, and in the end, we agreed to just stay with the current status quo. There is not really any action to take here at this point.