Make is_exterior_power (and friends) internal

[lgoettgens]

As this does return not only a bool, we should consider this (or a similar) rename. We can then, at a later point, introduce a new is_exterior_power function that only returns the bool.

I already did the same change for other types of power Lie algebra modules. This is not as important right now, as that code resides in experimental/. But exterior powers of modules are in src/. What do you think about this change @HechtiDerLachs ?

EDIT: In the below discussion it was decided to instead make the functions internal

[HechtiDerLachs]

Sorry, but I do not like this personally. Another example: divides returns (::Bool, ::RingElem) but noone ever considered calling this divides_with_data. If it is decided from some editorial board that we really want this, I will not stand in the way [*]. But at the same time, I do not see myself voluntarily approving this PR.

[*] If this was decided already, could you point me to where that decision is documented? Then I am happy to approve this PR.

Edit: Also, I consider this to be a rather internal function. So shortness of the name wins for me against name being all explanatory.

[lgoettgens]

There were some discussions about this time of functions in some of the past coding sprints (see the note here: https://hackmd.io/nRnyfrSxTVe5CzXidB1cBQ?both#functions-which-check-and-potentially-return-values), but there hasn't been a definite decision as far as I can remember.

Then maybe let us take this as a concrete example for the future of the broad set of is_* functions with multiple return values to discuss next Friday.

[codecov]

Codecov Report

Merging #3346 (0764e44) into master (a0913ed) will increase coverage by 0.02%.
Report is 14 commits behind head on master.
The diff coverage is 93.02%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3346      +/-   ##
==========================================
+ Coverage   81.82%   81.84%   +0.02%     
==========================================
  Files         556      556              
  Lines       74334    74361      +27     
==========================================
+ Hits        60822    60864      +42     
+ Misses      13512    13497      -15     

Files	Coverage Δ
experimental/LieAlgebras/src/LieAlgebraModule.jl	90.65% <100.00%> (ø)
experimental/LieAlgebras/src/LieAlgebras.jl	100.00% <ø> (ø)
...rimental/LieAlgebras/test/LieAlgebraModule-test.jl	100.00% <ø> (ø)
src/Modules/ExteriorPowers/FreeModules.jl	99.18% <100.00%> (ø)
...xperimental/LieAlgebras/src/LieAlgebraModuleHom.jl	85.63% <90.90%> (ø)
src/Modules/ExteriorPowers/Generic.jl	90.98% <92.30%> (ø)
src/Modules/deRhamComplexes.jl	89.51% <50.00%> (ø)

... and 7 files with indirect coverage changes

[fingolfin]

My take

• if is_exterior_power is an internal function, it should be renamed to e.g. _is_exterior_power and then can do whatever you want
• in general as I recall it, we did decide on this already quite some time ago; and also in recent discussions there was a general consensus on it (meaning as usual: not everybody was happy, but ultimately sufficiently many people agreed and nobody objected) that function named is_X should return a boolean and nothing else, otherwise they are confusing for users
• Magma has a concept were a function is informed whether the code calling it asks for 1 or 2 (etc.) return values, and can use that to cater what it does to the call site. In Julia we unfortunately don't have this feature
• regarding divides: well, we have is_divisible_by and divides could in theory be called is_divisible_by_with_data but it just isn't. And yeah one could reasonably argue that divides(a,b) also suggests a single return value and thus can be confusing, but we do have to make a cut somewhere; you can always find some precedent were OSCAR or Julia or both do something in favor or against whatever feature under debate. But at least the rule "functions named is_FOO should only return a single boolean value" is clear and easy to understand and follow

[fieker]

Consensus: bad names. This is only testing if an object was created this way. Maybe:

• change to is_created_as_...
• add a leading underscore
  A clear mathematical name should actually do math

[lgoettgens]

Should is_created_as_exterior_power then only supposed to return a bool or is it fine to return other stuff as well?

Anyway, I think my preference would then be to make these functions internal (I.e. Add a underscore and remove the export). Would this be fine with you (and your book chapter) @HechtiDerLachs?

[lgoettgens]

Anyway, I think my preference would then be to make these functions internal (I.e. Add a underscore and remove the export). Would this be fine with you (and your book chapter) @HechtiDerLachs?

Bump

[HechtiDerLachs]

Yes, that is OK for me. I can adjust this in my parts of the code then.

[lgoettgens]

I'll adapt this PR to reflect this

[lgoettgens]

Please have a look at the module code @HechtiDerLachs, the Lie algebra code is fine if the tests pass

[lgoettgens]

Please have a look at the module code @HechtiDerLachs, the Lie algebra code is fine if the tests pass

Bump

[HechtiDerLachs]

Looks fine. Thanks.