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

Fix HighestWeightModule for Lie algebras in certain cases #2617

Merged
merged 1 commit into from
Jul 6, 2018

Conversation

fingolfin
Copy link
Member

This is a regression in GAP 4.9 compared to GAP 4.8. As far as I can tell, it
is caused by the new sort functions; for some reason I don't quite
understand, the new sort ends up comparing an element to itself. But the
custom lexord sorting function used by HighestWeightModule was not
prepared to deal with two equal (much less identical) inputs.

Fixes #2491

I suggest to backport this to stable-4.9 -- it can wait for GAP 4.9.3, though, if adding it now is inconvenient.

This is a regression in GAP 4.9 compared to GAP 4.8. As far as I can tell, it
is caused by the new sort functions; for some reason I don't quite
understand, the new sort ends up comparing an element to itself. But the
custom `lexord` sorting function used by `HighestWeightModule` was not
prepared to deal with two equal (much less identical) inputs.

Fixes gap-system#2491
@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them regression A bug that only occurs in the branch, not in a release backport-to-4.9 labels Jul 4, 2018
@fingolfin
Copy link
Member Author

@ChrisJefferson I really wonder why the sorting function ends up comparing an element to itself (the input is free of duplicates; the comparison function really receives the same (identical) object twice as input.

@codecov
Copy link

codecov bot commented Jul 4, 2018

Codecov Report

Merging #2617 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2617      +/-   ##
==========================================
+ Coverage    74.8%   74.81%   +<.01%     
==========================================
  Files         479      479              
  Lines      242251   242193      -58     
==========================================
- Hits       181206   181185      -21     
+ Misses      61045    61008      -37
Impacted Files Coverage Δ
lib/lierep.gi 84.06% <100%> (+0.14%) ⬆️
src/stats.c 89.41% <0%> (-0.2%) ⬇️
grp/classic.gi 95.57% <0%> (-0.06%) ⬇️
grp/classic.gd 100% <0%> (ø) ⬆️
src/gasman.c 87.84% <0%> (ø) ⬆️
lib/stbcrand.gi 90.56% <0%> (+0.09%) ⬆️
hpcgap/lib/hpc/stdtasks.g 63.93% <0%> (+0.51%) ⬆️
src/calls.h 97.87% <0%> (+2.12%) ⬆️
src/funcs.c 97.63% <0%> (+7.73%) ⬆️

@fingolfin
Copy link
Member Author

Note that PR #2621 also fixes this issue, but I really want this one in, too, as arguably the comparator is wrong otherwise (we could have to equal but non-identical entries in a list being sorted, after all... OK, maybe not in the code being modified here, but...)

Copy link
Member

@sebasguts sebasguts left a comment

Choose a reason for hiding this comment

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

This looks fine to me, although I do not understand why it worked before . . .

@fingolfin
Copy link
Member Author

It worked before because the comparator never was called with two inputs that happened to be equal. But this changed when we changed our sorting algo.

@fingolfin fingolfin merged commit ab1e3fa into gap-system:master Jul 6, 2018
@fingolfin fingolfin deleted the mh/fix-HighestWeightModule branch July 6, 2018 23:40
@fingolfin fingolfin added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them kind: bug Issues describing general bugs, and PRs fixing them regression A bug that only occurs in the branch, not in a release 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.

3 participants