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.

Sign up for GitHub

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

Jump to bottom

[atlasrep] Update to 2.1.7 #821

Merged
merged 1 commit into from
Sep 1, 2023
Merged

[atlasrep] Update to 2.1.7 #821

merged 1 commit into from
Sep 1, 2023

Conversation

gap-package-distribution-bot[bot]
Copy link
Contributor

@gap-package-distribution-bot gap-package-distribution-bot bot added automated pr Automatically applied to PRs created by a GH workflow package update labels Aug 25, 2023
@gap-package-distribution-bot
Copy link
Contributor Author

gap-package-distribution-bot bot commented Aug 25, 2023 •
edited
Loading

Package Evaluation Report for GAP master

Job Properties

Testing: master/2023-09-01-21:43:35-9cb67296 vs master/2023-09-01-21:13:20-3ab20e58

Generated by Workflow: https://github.com/gap-system/PackageDistro/actions/runs/6054007103

In total, 156 packages were tested, out of which 151 succeeded, 1 failed and 4 were skipped.

❗ Packages still failing

1 package(s) failed tests also on the previous version.

✔️ Packages still succeeding

151 package(s) succeeded tests also on the previous version.

Click to show packages!

➖ Packages that were skipped

4 package(s) skipped tests also on the previous version.

Click to show packages!

@fingolfin
Copy link
Member

@ThomasBreuer apparently the new AtlasRep creates a read-only variable ZZ which breaks the CAP/homalg test suites...

@ThomasBreuer
Copy link
Collaborator

@fingolfin
ZZ is a (documented) operation from the StandardFF package.
AtlasRep version 2.1.7 introduces a new dependency to StandardFF as a suggested package.

@fingolfin
Copy link
Member

OK. I guess we can ask @zickgraf if they can avoid using ZZ in their test suite as a free variable.

@zickgraf
Copy link

zickgraf commented Sep 1, 2023

I have created homalg-project/CAP_project#1455 for CAP_project. Ping @mohamed-barakat for homalg_project.

However, I think it is not very polite of packages (or of GAP itself) to just "occupy" those short names like Z, ZZ, E, EE etc. Of course this makes it very convenient to write code using those functions, but I am always annoyed when trying to do something like X := ...; Y := ...; Z := ...; in the REPL, where X and Z are already occupied. My workaround usually was XX := ...; YY := ...; ZZ := ...;, which now also breaks. I would much prefer if packages would use longer names. If one really uses those functions excessively, one could always create a local alias with a short name, without polluting the global namespace.

@ThomasBreuer
Copy link
Collaborator

@zickgraf

  • If the question is about testfiles then one can put a #@local statement into the testfile in order to declare variables used later on as local. For example, the following testfile would work, no matter whether the StandardFF package is loaded or not.
 #@local Z, ZZ
gap> START_TEST("test.tst");;
gap> Z:= 1;
1
gap> ZZ:= 1;
1
gap> Z = ZZ;
true
gap> STOP_TEST("test.tst");;
  • There is a statement in the GAP Tutorial that

The name of every global variable in the GAP library starts with a capital letter. Thus if you choose only names starting with a small letter for your own variables you will not attempt to overwrite any predefined variable. (Note that most of the predefined variables are read-only, and trying to change their values will result in an error message.)

Thus the suggestion is to use x, y, z, etc. instead of X, Y, Z, etc. as user variables.
Yes, this statement is well-hidden; I had thought that there is another instance somewhere in the Reference Manual, but I did not find it now.

@fingolfin
Copy link
Member

I agree with both of you: it is annoying that X, E, Z, ..., and now ZZ are "blocked"; but also there is a clear rule to avoid this ("don't use variables starting with an upper case letter"), and a simple workaround for test files, too (namely using #@local -- though it has the drawback that these examples then are not trivial to reproduce interactively).

Anyway, thanks @zickgraf for preparing a patch, and it seems this was already released, great. To be on the safe side long term, you may wish to rename ZZZ to zz or so... but for now it is of course perfectly fine 😃)

@mohamed-barakat
Copy link
Member

but also there is a clear rule to avoid ("don't use variables starting with an upper case letter")

Grepping in gap/lib for "G:=", "U:=" one finds not only occurrences as local variables in the code but also in the GAPDoc sections, but maybe this GAPDoc code is not tested. So I assume the rule is a strong recommendation.

I am aware that these issues are consequences of GAP missing the concept of namespaces and that it would be a considerable amount of work to introduce them, so I am not complaining.

I will try to replace " ZZ" with " zz" in the homalg packages and see what happens.

mohamed-barakat added a commit to mohamed-barakat/homalg_project that referenced this pull request Sep 1, 2023
@mohamed-barakat
renamed variable ZZ -> zz in the tested documentation examples
70b62dc 
see gap-system/PackageDistro#821 (comment)
@mohamed-barakat mohamed-barakat mentioned this pull request Sep 1, 2023
Merged
mohamed-barakat added a commit to mohamed-barakat/homalg_project that referenced this pull request Sep 1, 2023
@mohamed-barakat
renamed variable ZZ -> zz in the tested documentation examples
b5a2744 
see gap-system/PackageDistro#821 (comment)
mohamed-barakat added a commit to mohamed-barakat/homalg_project that referenced this pull request Sep 1, 2023
@mohamed-barakat
renamed variable ZZ -> zz in the tested documentation examples
9b911fe 
see gap-system/PackageDistro#821 (comment)
@fingolfin
Copy link
Member

The CAP fixes by @zickgraf helped but of course we still have failures in various homalg packages, so waiting for new release of these:

  • homalg 2023.08-01
  • localizeringforhomalg 2023.08-01
  • matricesforhomalg 2023.08-01
  • modules 2023.08-01
  • ringsforhomalg 2023.08-01

mohamed-barakat added a commit to mohamed-barakat/homalg_project that referenced this pull request Sep 1, 2023
@mohamed-barakat
renamed variable ZZ -> zz in the tested documentation examples
1260d01 
see gap-system/PackageDistro#821 (comment)
@fingolfin fingolfin merged commit 4a4081d into main Sep 1, 2023
308 of 310 checks passed
@fingolfin fingolfin deleted the automatic/atlasrep branch September 1, 2023 22:19
@mohamed-barakat
Copy link
Member

Our CIs are failing although they are not tested with LoadAllPackages( ). This probably means that standard GAP is now marking ZZ read-only. Is there a way to undo this change in the near future?

Side note: QPA2 is now also failing due to a conflict in the declaration of AsVector.

@fingolfin
Copy link
Member

I don't understand, I thought you replaced all uses of ZZ?

@mohamed-barakat
Copy link
Member

Yes, we did in the deposited packages. I assumed this would be enough :)

@ThomasBreuer
Copy link
Collaborator

@mohamed-barakat

This probably means that standard GAP is now marking ZZ read-only. Is there a way to undo this change in the near future?

I do not understand this statement.
The operation ZZ belongs to the StandardFF package.
It is bound and then readonly as soon as this package is loaded.
Making ZZ in StandardFF not readonly would be wrong.

The clash with user variables in CAP/homalg packages occurs since AtlasRep suggests loading StandardFF,
thus I guess that "standard GAP" means GAP with some packages, one of them being AtlasRep.
If one really wants to avoid loading StandardFF, one could try to load AtlasRep explicitly with the OnlyNeeded option, but for that one has to make sure that AtlasRep has not been loaded before.

@mohamed-barakat
Copy link
Member

The clash with user variables in CAP/homalg packages occurs since AtlasRep suggests loading StandardFF, thus I guess that "standard GAP" means GAP with some packages, one of them being AtlasRep.

Yes.

I do not understand this statement. The operation ZZ belongs to the StandardFF package. It is bound and then readonly as soon as this package is loaded. Making ZZ in StandardFF not readonly would be wrong.

I was not suggesting making ZZ read/write. I was asking if it would be possible to rename ZZ in StandardFF into something different :) If the answer is no we have to rename all occurrences of ZZ into something else in our non-deposited packages as well.

@mohamed-barakat
Copy link
Member

In the meanwhile, I have looked into StandardFFand now understand the logic behind using ZZ.

@zickgraf
Copy link

zickgraf commented Sep 4, 2023

Side note: QPA2 is now also failing due to a conflict in the declaration of AsVector.

See frankluebeck/StandardFF#6 for the clash of "AsVector" with QPA2. For now I will simply delete StandardFF from our CI completely.

zickgraf added a commit to homalg-project/gap-docker that referenced this pull request Sep 4, 2023
@zickgraf
Remove StandardFF
6a5eebb 
We do not need it but it causes various issues,
see gap-system/PackageDistro#821
and frankluebeck/StandardFF#6
zickgraf added a commit to homalg-project/gap-docker-master that referenced this pull request Sep 4, 2023
@zickgraf
Remove StandardFF
deee7d1 
We do not need it but it causes various issues,
see gap-system/PackageDistro#821
and frankluebeck/StandardFF#6
zickgraf added a commit to homalg-project/gap-docker-master that referenced this pull request Sep 4, 2023
@zickgraf
Remove StandardFF
c396e67 
We do not need it but it causes various issues,
see gap-system/PackageDistro#821
and frankluebeck/StandardFF#6
@fingolfin
Copy link
Member

Unfortunately QPA2 is not a deposited package and hence clashes in it with deposited packages are not detected. So in this case I am afraid the onus is on QPA2 maintainers to work something out with the StandardFF maintainer (Frank Lübeck) and/or to change their package.

ThomasBreuer added a commit to ThomasBreuer/gap that referenced this pull request Sep 6, 2023
@ThomasBreuer
added a rule about variable names
82b5953 
In the discussion of gap-system/PackageDistro#821,
it was noticed that this rule was apparently stated only in the Tutorial.

Now it goes also to the Reference Manual, hopefully this is easier to find.
@ThomasBreuer ThomasBreuer mentioned this pull request Sep 6, 2023
Merged
fingolfin pushed a commit to gap-system/gap that referenced this pull request Sep 6, 2023
@ThomasBreuer
added a rule about variable names (#5505)
2ec693a 
In the discussion of gap-system/PackageDistro#821,
it was noticed that this rule was apparently stated only in the Tutorial.

Now it goes also to the Reference Manual, hopefully this is easier to find.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
automated pr Automatically applied to PRs created by a GH workflow package update
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fingolfin @ThomasBreuer @zickgraf @mohamed-barakat