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

Make is_exterior_power (and friends) internal #3346

Merged
merged 4 commits into from
Feb 20, 2024

Conversation

lgoettgens
Copy link
Member

@lgoettgens lgoettgens commented Feb 9, 2024

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

Copy link
Collaborator

@HechtiDerLachs HechtiDerLachs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

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.

Copy link

codecov bot commented Feb 9, 2024

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
Copy link
Member

fingolfin commented Feb 13, 2024

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
Copy link
Contributor

fieker commented Feb 14, 2024

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Collaborator

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

@lgoettgens
Copy link
Member Author

I'll adapt this PR to reflect this

@lgoettgens lgoettgens removed the triage label Feb 16, 2024
@lgoettgens lgoettgens added the backport 1.0.x Should be backported to the release 1.0 branch label Feb 16, 2024
@lgoettgens
Copy link
Member Author

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

@lgoettgens lgoettgens changed the title Rename is_exterior_power to is_exterior_power_with_data (and friends) Make is_exterior_power (and friends) internal Feb 16, 2024
@benlorenz benlorenz mentioned this pull request Feb 16, 2024
33 tasks
@lgoettgens
Copy link
Member Author

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

Bump

Copy link
Collaborator

@HechtiDerLachs HechtiDerLachs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine. Thanks.

@benlorenz benlorenz merged commit 9d88a8f into oscar-system:master Feb 20, 2024
20 checks passed
@lgoettgens lgoettgens deleted the lg/is_exterior_power branch February 21, 2024 07:35
benlorenz pushed a commit that referenced this pull request Feb 21, 2024
* Rename `is_exterior_power` to `is_exterior_power_with_data` (and friends)

* Revert "Rename `is_exterior_power` to `is_exterior_power_with_data` (and friends)"

This reverts commit b19369b.

* Make `is_exterior_power` internal

* the same for similar functions

(cherry picked from commit 9d88a8f)
@benlorenz benlorenz removed the backport 1.0.x Should be backported to the release 1.0 branch label Feb 21, 2024
benlorenz added a commit that referenced this pull request Feb 23, 2024
### Backported PRs

- [x] #3367
- [x] #3379 
- [x] #3369
- [x] #3291
- [x] #3325
- [x] #3350 
- [x] #3351
- [x] #3365 
- [x] #3366
- [x] #3382
- [x] #3373
- [x] #3341
- [x] #3346
- [x] #3381
- [x] #3385
- [x] #3387 
- [x] #3398 
- [x] #3399 
- [x] #3374 
- [x] #3406 
- [x] #2823
- [x] #3298
- [x] #3386 
- [x] #3412 
- [x] #3392 
- [x] #3415 
- [x] #3394
- [x] #3391
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants