Skip to content

Add LINS to gap_packages #39783

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

Merged
merged 10 commits into from
Apr 29, 2025
Merged

Add LINS to gap_packages #39783

merged 10 commits into from
Apr 29, 2025

Conversation

enriqueartal
Copy link
Contributor

There is a new package gap package LINS and the goal of this PR is to add it to gap_packages. There is another change (which is not essential): allow parallel computation, at least of each package. I wonder also if it would be interesting to add more packages, they are loaded only if needed.

I encountered an unsolved problem. I can use the package function LowIndexNormalSubs using libgap.function_factory. There are two entries: a group and a positive integer and the output is the list of all normal subgroups with index bounded by the integer if the option allSubgroups is true (default value); if false, only the subgroups with exact index are obtained and if it is the goal, it is much faster than obtaining the whole list and filtering. But I do not know how to pass an option using function_factory, and I do not know how to pass the group using libgap_eval.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Copy link

github-actions bot commented Mar 24, 2025

Documentation preview for this PR (built with commit 6169d51; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@dimpase
Copy link
Member

dimpase commented Mar 26, 2025

independent package can be built in parallel. With GAP packages interdependencies, one has to be more careful

@enriqueartal
Copy link
Contributor Author

Thanks, should the label be changed?

@dimpase
Copy link
Member

dimpase commented Mar 29, 2025

yes, please change the label

@enriqueartal
Copy link
Contributor Author

Is there a reason for removing the positive review label?

@dimpase
Copy link
Member

dimpase commented Apr 6, 2025

this happens automatically after a git push

vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 20, 2025
sagemathgh-39783: Add LINS to gap_packages
    
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->

There is a new package gap package [LINS](https://github.com/gap-
packages/LINS) and the goal of this PR is to add it to `gap_packages`.
There is another change (which is not essential): allow parallel
computation, at least of each package. I wonder also if it would be
interesting to add more packages, they are loaded only if needed.

I encountered an unsolved problem. I can use the package function
`LowIndexNormalSubs` using `libgap.function_factory`. There are two
entries: a group and a positive integer and the output is the list of
all normal subgroups with index bounded by the integer if the option
`allSubgroups`  is `true` (default value); if `false`, only the
subgroups with exact index are obtained and if it is the goal, it is
much faster than obtaining the whole list and filtering. But I do not
know how to pass an option using `function_factory`, and I do not know
how to pass the group using `libgap_eval`.


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [X] The title is concise and informative.
- [X] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39783
Reported by: Enrique Manuel Artal Bartolo
Reviewer(s): Dima Pasechnik
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 21, 2025
sagemathgh-39783: Add LINS to gap_packages
    
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->

There is a new package gap package [LINS](https://github.com/gap-
packages/LINS) and the goal of this PR is to add it to `gap_packages`.
There is another change (which is not essential): allow parallel
computation, at least of each package. I wonder also if it would be
interesting to add more packages, they are loaded only if needed.

I encountered an unsolved problem. I can use the package function
`LowIndexNormalSubs` using `libgap.function_factory`. There are two
entries: a group and a positive integer and the output is the list of
all normal subgroups with index bounded by the integer if the option
`allSubgroups`  is `true` (default value); if `false`, only the
subgroups with exact index are obtained and if it is the goal, it is
much faster than obtaining the whole list and filtering. But I do not
know how to pass an option using `function_factory`, and I do not know
how to pass the group using `libgap_eval`.


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [X] The title is concise and informative.
- [X] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39783
Reported by: Enrique Manuel Artal Bartolo
Reviewer(s): Dima Pasechnik
@dimpase
Copy link
Member

dimpase commented Apr 25, 2025

is the sage-devel message


[email protected]

I found a way to pass options to functions in gap (looking at the use of function_factory in permgroup.py); I guess it can be improved and I do not know how to make it more useful. I put an example. In LINS package there is a function called LowIndexNormalSubs; without options, applied to a group G and a number n, it gives all the normal subgroups of G of index at most n; if one is only interested in n-index subgroups one has to pass the option allSubgroups as false; if only these subgroups are needed it is much faster than computing all of them and filter. The following seams to work

low_index_normal_subgroups_exact = libgap.function_factory("""function(G, n) return LowIndexNormalSubs(G, n : allSubgroups := false); end; """)

relevant here?

@enriqueartal
Copy link
Contributor Author

I think not directly. Till now I was unable to pass the option to this function in sage. I think it is not bad to have predefined sage functions for functions in gap, but I am not sure neither which ones nor how. Maybe it could be addressed in another PR.

@dimpase
Copy link
Member

dimpase commented Apr 25, 2025

in a way, pre-defined GAP functions are there.
Namely, if foo(x,y) is a GAP function you want to pass already known to Sage objects, then
x.foo(y) is a valid Sage function.
As well as libgap.foo(x,y) is good, too.
(needless to say, it might be that in some cases one needs libgap(x) rather than x)

You can see this used quite a bit in e.g.
src/sage/graphs/generators/classical_geometries.py.

Thus, I am not sure what exactly you are missing.

@vbraun vbraun merged commit d0ed6df into sagemath:develop Apr 29, 2025
19 of 32 checks passed
@slel
Copy link
Member

slel commented May 3, 2025

This contribution added two files Pipfile and src/Pipfile.
These (autogenerated?) files used to be "git ignored" until #39031.
Should a followup "git remove" them and "git ignore" them again?
See also:

@dimpase
Copy link
Member

dimpase commented May 3, 2025

@enriqueartal - where these Pipfiles added by you here a mistake?
I thought this PR needs them. If not, we should just purge them.

@enriqueartal
Copy link
Contributor Author

It is a mistake. I sincerely do not know why they are there. Since it is closed, should I do something?

@dimpase
Copy link
Member

dimpase commented May 3, 2025

It is a mistake. I sincerely do not know why they are there.
from using git commit -a without carefully looking what files you are adding, I suppose

Since it is closed, should I do something?

@enriqueartal open a PR which removes them, I'll approve it straight away

@enriqueartal enriqueartal mentioned this pull request May 3, 2025
5 tasks
@enriqueartal
Copy link
Contributor Author

Done, I hope correctly. Sorry for the inconvenience

vbraun pushed a commit to vbraun/sage that referenced this pull request May 4, 2025
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

A couple of Pipfile's were added by mistake in the branch of sagemath#39783.
They are removed here.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [X] The title is concise and informative.
- [X] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

URL: sagemath#40044
Reported by: Enrique Manuel Artal Bartolo
Reviewer(s): Dima Pasechnik
vbraun pushed a commit to vbraun/sage that referenced this pull request May 5, 2025
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

A couple of Pipfile's were added by mistake in the branch of sagemath#39783.
They are removed here.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [X] The title is concise and informative.
- [X] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

URL: sagemath#40044
Reported by: Enrique Manuel Artal Bartolo
Reviewer(s): Dima Pasechnik
vbraun pushed a commit to vbraun/sage that referenced this pull request May 6, 2025
sagemathgh-40044: Remove Pipfiles
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

A couple of Pipfile's were added by mistake in the branch of sagemath#39783.
They are removed here.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [X] The title is concise and informative.
- [X] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#40044
Reported by: Enrique Manuel Artal Bartolo
Reviewer(s): Dima Pasechnik
vbraun pushed a commit to vbraun/sage that referenced this pull request May 9, 2025
sagemathgh-40044: Remove Pipfiles
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

A couple of Pipfile's were added by mistake in the branch of sagemath#39783.
They are removed here.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [X] The title is concise and informative.
- [X] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#40044
Reported by: Enrique Manuel Artal Bartolo
Reviewer(s): Dima Pasechnik
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants