-
Notifications
You must be signed in to change notification settings - Fork 165
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
Add and document new function DirectProductFamily
#3455
Add and document new function DirectProductFamily
#3455
Conversation
I have added the |
Several tests failed due to a |
af39b21
to
2c4928e
Compare
Codecov Report
@@ Coverage Diff @@
## master #3455 +/- ##
==========================================
+ Coverage 85.4% 85.63% +0.22%
==========================================
Files 696 646 -50
Lines 344610 319019 -25591
==========================================
- Hits 294327 273203 -21124
+ Misses 50283 45816 -4467
|
Thanks for the heads-up. I fixed the issue. The filter |
Do you want to be able to call this in your own code? If so, we should document it, so we know what it is supposed to do. |
Tests for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to document your code, I don't see any reason against it?
In documenting this, (and
Arguably, perhaps they should be, with |
Good point! Thanks |
I was thinking about doing a function |
9070d68
to
823999a
Compare
I've updated the PR! |
DirectProductFamily
823999a
to
49acf72
Compare
Added the method to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems plausible to me.
Looks good to me. |
49acf72
to
a715bed
Compare
a3befc2
to
fc8b876
Compare
I rebased onto master and put the new changes into a separate commit. Once this PR is accepted I'll squash everything into a single commit. |
fc8b876
to
0911e3e
Compare
Squashed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this looks pretty good now! Unfortunately there is one issue left, but that should be easy to fix: changes to lib/mapping.gi
also need to be applied to hpcgap/lib/mapping.gi
.
This declares the new operation DirectProductFamily and installs a method for it. The new method is also added to `hpcgap/lib/tuples.gi`. Also adds documentation and tests. In the reference manual, the section "IsDirectProductElement (Filter)" is renamed into "Direct Products and their Elements" to also accomodate the documentation of DirectProductFamily and future categories and operations. UnderlyingRelation is adapted to use the new method, which makes it easier to read. The same change is replicated to `hpcgap/lib/mapping.gi`.
0911e3e
to
4ea0738
Compare
lib: add operation DirectProductFamily
This declares the new operation DirectProductFamily and installs a
method for it. Also adds documentation and tests.
In the reference manual, the section "IsDirectProductElement (Filter)"
is renamed into "Direct Products and their Elements" to also accomodate
the documentation of DirectProductFamily and future categories and
operations.
UnderlyingRelation is adapted to use the new method, which makes it
easier to read.