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

WIP: Constructors #611

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

stevelinton
Copy link
Contributor

So far this PR fixes the immediate problem reported in #601 that method selection for constructors can miss applicable methods.

There are still a number of things to do in this area:

  1. Work out what method should be preferred for a constructor when several are applicable?
    For instance what kind of group should AbelianGroupCons(IsGroup,[0]) return?
    and make the method selection do it.
  2. Document constructors
  3. Possible add a more efficient IS_CONSTUCTOR (might wait until ObjSets are merged because that would be a perfect solution.
  4. Simplify code that works around issue Constructors #601, such as AbelianGroup

@markuspf markuspf self-assigned this Jul 5, 2016
@markuspf
Copy link
Member

markuspf commented Jul 5, 2016

Working with people in Bremen on OpenDreamKit's WP 6, I have some patches for constructors as well. I will have a look and coordinate with @stevelinton on this.

@fingolfin
Copy link
Member

What is the status of this? Do you plan to resume work on this?

@stevelinton
Copy link
Contributor Author

I'll try and catch up with Markus next week and work out a plan

@markuspf
Copy link
Member

I think most of the code that I (thought?) I had has been merged in the various type information pull requests I made, and I believe the stuff I thought about is slightly different from what goes under Constructor here.

I do remember though that @ssicha, @sebasguts, @frankluebeck, and me had a discussion about constructors (in the GAP sense) when working on MatrixObjs in Aachen, and some documentation might have been produced in the process.

For completeness: What we were doing for ODK WP6 was trying to figure out how to produce for any object in a running GAP session how it had been constructed by applying a function (which we called a data constructor) to some arguments. There is some existing code that does this in my github repository that was written by an intern and tries to deduce from the surroundings of a call to Objectify how an object was constructed. It works well enough for the ODK WP6 experiments, but will need some serious work to be usable for mainline GAP (in particular it slows down GAP by quite a margin).

@ssiccha
Copy link
Contributor

ssiccha commented Sep 14, 2017

Yes, I documented the behaviour of constructors in #1391.

@fingolfin fingolfin added the status: awaiting response Issues and PRs whose progress is stalled awaiting a response from (usually) the author label Jun 12, 2018
@fingolfin
Copy link
Member

@stevelinton What's the status of this? At the very least it needs a rebase; but it's also unclear to me whether this is something that is planned to be revived one day or not?

@stevelinton
Copy link
Contributor Author

@fingolfin issue #601 is still valid, I think, but the final comment there comes after my work on this PR and essentially says "this isn't as simple as I thought". So I guess the correct thing to do is pick the "obviously correct" bits of this PR, not already taken care of by @sebasguts, if any, rebase them and get them in, and then close this PR but leave the issue open. We should also try and remember to discuss it at the next GAP Days or skype.

@stevelinton stevelinton added do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) kind: bug Issues describing general bugs, and PRs fixing them topic: library kind: discussion discussions, questions, requests for comments, and so on labels Jun 12, 2018
@wilfwilson
Copy link
Member

@fingolfin @stevelinton This is currently the only open PR/issue with the label status: awaiting response. Since #3366 has been merged, I believe that this means that the PR will be automatically closed soon. Is this label still appropriate?

@fingolfin fingolfin removed the status: awaiting response Issues and PRs whose progress is stalled awaiting a response from (usually) the author label Mar 25, 2019
@fingolfin
Copy link
Member

I removed that label, as there was a response, although I missed it, and we did not discuss this on either of the two GAP days since @stevelinton's remark...

@fingolfin
Copy link
Member

I have rebased this PR locally, thus resolving conflicts and reducing it to three commits (the other four were already in master). @stevelinton if you activate the "Allow edits from maintainers" checkbox, then I can also update this PR for you; alternatively, you can get the rebased branch from https://github.com/fingolfin/gap/tree/constructors

@stevelinton
Copy link
Contributor Author

Ticked that box. Thanks

@fingolfin
Copy link
Member

... and force pushed the rebased PR

@codecov
Copy link

codecov bot commented May 28, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@5fbcf2b). Click here to learn what that means.
The diff coverage is 95%.

@@            Coverage Diff            @@
##             master     #611   +/-   ##
=========================================
  Coverage          ?   48.29%           
=========================================
  Files             ?      108           
  Lines             ?    46030           
  Branches          ?        0           
=========================================
  Hits              ?    22229           
  Misses            ?    23801           
  Partials          ?        0
Impacted Files Coverage Δ
src/c_oper1.c 66.63% <95%> (ø)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) kind: bug Issues describing general bugs, and PRs fixing them kind: discussion discussions, questions, requests for comments, and so on topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants