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

Immutable entries of mutable attributes of char. tables #2657

Merged
merged 1 commit into from
Aug 11, 2018

Conversation

ThomasBreuer
Copy link
Contributor

Character tables use several mutable attributes whose values are
extendible lists:
ComputedClassFusions, ComputedIndicators, ComputedPowerMaps,
ComputedPrimeBlockss.

Up to now, the entries of these lists were mutable.
@frankluebeck suggested to make these entries immutable.
This change makes sense.

Note:
The change may have side-effects w.r.t. users' GAP code.
For example, the object returned by

0 * ComputedPowerMaps( CharacterTable( "A5" ) )[2]`

had been a mutable list before the changes,
and will be an immutable list after the changes.

In order to achieve the immutability,
the following changes were necessary.

  • Extended SupportedCharacterTableInfo and DeclareAttributeSuppCT
    such that the mutability of the attribute in question is recorded;
    there seems to be no way (and no need) to access this information
    from the attribute itself,

  • changed DeclarePropertySuppCT, here mutability makes no sense,

  • let ConvertToCharacterTableNC make the entries of mutable attributes
    immutable.

Other changes in the context of mutability of attributes for character tables:

  • Removed the DeclareAttributeSuppCT call for InfoText;
    meanwhile this attribute is declared for arbitrary GAP objects,
    redeclaring it for special objects is logically wrong,

  • call MakeImmutable for (entries of) attribute values before the
    conversion of a record to a character table object,
    in order to avoid the creation of a copy in the standard process of
    setting attribute values,

  • documented some optional components of an argument for
    ContainedPossibleCharacters.

Copy link
Member

@frankluebeck frankluebeck left a comment

Choose a reason for hiding this comment

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

All these changes look sensible and useful. As mentioned by the author of this pull request, there is a small possibility that code which worked before is broken. But as far as I can see such code is probably buggy anyway, and it will always be easy to fix/correct it.

I have no idea why one of the tests fails, this seems to have to do with the Float package and I cannot see any relation with the changes in this pull request.

The only further change I would recommend is to add a test in some 'tst/testinstall/ctbl*' file, checking immutability of stored powermaps in some table.

@markuspf
Copy link
Member

@frankluebeck @ThomasBreuer the test failures were due to the package float which we have disabled right now, as debugging this issue is not on our list of priorities.

@codecov
Copy link

codecov bot commented Jul 26, 2018

Codecov Report

Merging #2657 into master will increase coverage by <.01%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master    #2657      +/-   ##
==========================================
+ Coverage   75.41%   75.41%   +<.01%     
==========================================
  Files         478      478              
  Lines      241578   241606      +28     
==========================================
+ Hits       182192   182218      +26     
- Misses      59386    59388       +2
Impacted Files Coverage Δ
lib/ctblgrp.gi 84.62% <100%> (ø) ⬆️
lib/ctblmaps.gi 70.54% <17.64%> (+0.05%) ⬆️
lib/ctblsymm.gi 29.77% <28.57%> (-0.02%) ⬇️
lib/ctblfuns.gi 60.52% <66.66%> (-0.02%) ⬇️
lib/ctbl.gi 71.91% <86.3%> (+0.09%) ⬆️
lib/ctbl.gd 75.86% <90.47%> (+5.02%) ⬆️
hpcgap/lib/hpc/stdtasks.g 64.96% <0%> (+0.51%) ⬆️

@fingolfin fingolfin added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Aug 1, 2018
@fingolfin
Copy link
Member

@frankluebeck you "requested changes", which blocks merging of this PR. But in what you write, I don't see any changes you "request" -- only a suggestion for some changes.

Could you either drop the "request for changes" (using the "Dismiss review" link on the PR page), or else clarify what the requested changes are?

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.

The explanation and the changes seem plausibel and sensible to me. Caveat: I am not familiar with the actual, though. But I think Thomas is the expert on it, so I am happy to trust his judgement here.

Character tables use several mutable attributes whose values are
extendible lists:
`ComputedClassFusions`, `ComputedIndicators`, `ComputedPowerMaps`,
`ComputedPrimeBlockss`.

Up to now, the entries of these lists were mutable.
@fluebeck suggested to make these entries immutable.
This change makes sense.

*Note*:
The change may have side-effects w.r.t. users' GAP code.
For example, the object returned by
```
0 * ComputedPowerMaps( CharacterTable( "A5" ) )[2]`
```
had been a *mutable* list before the changes,
and will be an *immutable* list after the changes.

In order to achieve the immutability,
the following changes were necessary.

- Extended `SupportedCharacterTableInfo` and `DeclareAttributeSuppCT`
  such that the mutability of the attribute in question is recorded;
  there seems to be no way (and no need) to access this information
  from the attribute itself,

- changed `DeclarePropertySuppCT`, here mutability makes no sense,

- let `ConvertToCharacterTableNC` make the entries of mutable attributes
  immutable.

Other changes in the context of mutability of attributes for character tables:

- Removed the `DeclareAttributeSuppCT` call for `InfoText`;
  meanwhile this attribute is declared for arbitrary GAP objects,
  redeclaring it for special objects is logically wrong,

- call `MakeImmutable` for (entries of) attribute values before the
  conversion of a record to a character table object,
  in order to avoid the creation of a copy in the standard process of
  setting attribute values,

- documented some optional components of an argument for
  `ContainedPossibleCharacters`.

- added a few tests of the (im)mutability
@ThomasBreuer
Copy link
Contributor Author

I have now added some tests of (im)mutability, as suggested by @frankluebeck.
This was a useful hint.

@fingolfin fingolfin merged commit 01c8224 into gap-system:master Aug 11, 2018
@fingolfin
Copy link
Member

We need to describe this change for the release notes (see https://github.com/gap-system/gap/wiki/GAP-4.10-release-notes for the current draft), but I am not sure what to write, as this seems to mix various changes. @ThomasBreuer if you could provide a brief draft, that'd be great.

@ThomasBreuer
Copy link
Contributor Author

@fingolfin I have added an item to the release notes.

@fingolfin
Copy link
Member

Thank you!

@fingolfin fingolfin 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 Oct 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants