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

Key dependent operation #514

Merged
merged 3 commits into from
Jan 22, 2016
Merged

Conversation

bh11
Copy link
Contributor

@bh11 bh11 commented Jan 21, 2016

add setter and tester operations for KeyDependentOperations

This should make it easier to handle the problems raised in #412 and #511.

In the long run, it might be a good idea to "undocument" the Computed… attribute itself, and maybe replace it by a dictionary object.

This PR also fixes a potential problem when computing a value, the Computed… attribute gets changed (because the called method computed other attribute values on the way, etc).

Probably just paranoid, but the call to oper may have changed `known', or even stored the required information.
@bh11
Copy link
Contributor Author

bh11 commented Jan 21, 2016

P.S. I might also add a binary search, somehow hurts to see a linear search in a sorted list.

@markuspf
Copy link
Member

This seems like a prime application for the caching framework that @fingolfin was suggesting (also in view to HPC-GAP merge)

@hungaborhorvath
Copy link
Contributor

Ah, so the syntax would be

HasSylowSubgroup(G, 2); 

and

SetSylowSubgroup(G, 2, subgroup);

Is that right?

@hungaborhorvath
Copy link
Contributor

BTW, I find these as excellent and very useful functions, thank you very much for the effort.

Have not tested it, yet, though.

@bh11
Copy link
Contributor Author

bh11 commented Jan 21, 2016

@hungaborhorvath Yes, that's the syntax. There's an example in the manual section.

@fingolfin
Copy link
Member

This is a very nice idea. I also agree with @bh11's remarks on the Computed... attributes. As to the binary search: This is implemented by PositionSorted resp. PositionSet, perhaps they can be used (or perhaps they are already? I have not yet looked at the patch here itself, sorry).

UPDATE: I just noticed that there IS already an example (and thus test case) in the manual, so please disregard my bogus prior comment

@bh11 bh11 force-pushed the key-dependent-operation branch 3 times, most recently from f74fa7b to 63e0e10 Compare January 22, 2016 10:25
bh11 added a commit that referenced this pull request Jan 22, 2016
add tester and setter operations for key dependent operation

This also adds a binary search for stored attributes and fixes a potential problem when the underlying operation changes the mutable attribute.
@bh11 bh11 merged commit 069e46d into gap-system:master Jan 22, 2016
@bh11 bh11 added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Jan 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants