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

Preserve more properties in DirectProduct #3053

Merged
merged 2 commits into from
Nov 26, 2018

Conversation

wilfwilson
Copy link
Member

The soon-to-be-released Semigroups 3.1.0 is going to prove much greater support for creating direct products of semigroups. It does this by installing methods for DirectProductOp for certain kinds of semigroups, which uses the DirectProduct function in the GAP library. When this library function has created a direct product, it tries to see whether it can set some properties that are preserved by direct products and that are known for the factors. So I thought it made sense to increase this list of properties for when the arguments are semigroups.

What do people think? Is this going to slow things down noticeably for groups, where all of these properties are irrelevant? Is there a better way of doing this? Or is it a bad idea?

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Fine by me (although a few trivial tests to verify this works as intended would be kinda nice...)

@codecov
Copy link

codecov bot commented Nov 25, 2018

Codecov Report

Merging #3053 into master will decrease coverage by 8.99%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3053      +/-   ##
==========================================
- Coverage   83.79%    74.8%      -9%     
==========================================
  Files         685      629      -56     
  Lines      343386   290994   -52392     
==========================================
- Hits       287737   217664   -70073     
- Misses      55649    73330   +17681
Impacted Files Coverage Δ
lib/gprd.gi 57.92% <100%> (-13.98%) ⬇️
src/fibhash.h 0% <0%> (-100%) ⬇️
lib/nilpquot.gi 11.11% <0%> (-88.89%) ⬇️
lib/meatauto.gi 6% <0%> (-88.7%) ⬇️
src/modules.h 16.66% <0%> (-83.34%) ⬇️
lib/helpt2t.gi 0.23% <0%> (-83.14%) ⬇️
src/dteval.c 3.43% <0%> (-79.99%) ⬇️
src/objset.c 7.92% <0%> (-77.2%) ⬇️
lib/dt.g 7.75% <0%> (-72.2%) ⬇️
src/pperm.c 28.35% <0%> (-69.5%) ⬇️
... and 400 more

@wilfwilson
Copy link
Member Author

I agree @fingolfin, I'm all for tests adding tests and I agree it would be nice to have comprehensive tests for the stuff I've added. The difficulty here is that DirectProduct is declared in the GAP library, while the methods for DirectProductOp for constructing direct products of semigroups will be in the Semigroups package. So it will not be possible to test the new things that I have added. I can add such tests in the Semigroups package. Unfortunate, yes, but I don't see a way around this at the moment.

To make you a bit happier I have added a couple of tests relating to the properties that were already implemented for groups :)

@wilfwilson
Copy link
Member Author

I've added another commit because I had an idea of how to improve this further. So I suggest that this this should be reviewed again.

Explanation of change: the properties of direct products that are implemented all hold of the direct product if and only if they hold for all of the factors. Therefore, to set the property to be false for the direct product, we only need to know that the property is false for one of the factors.

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Thanks!

@fingolfin fingolfin merged commit aac1de6 into gap-system:master Nov 26, 2018
@wilfwilson wilfwilson deleted the update-direct-product branch November 26, 2018 12:10
@fingolfin fingolfin added kind: discussion discussions, questions, requests for comments, and so on and removed kind: request for comments labels Mar 21, 2019
@PaulaHaehndel PaulaHaehndel added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Apr 15, 2019
@wilfwilson wilfwilson added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Apr 24, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: discussion discussions, questions, requests for comments, and so on release notes: added PRs introducing changes that have since been mentioned in the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants