use ACE 4.1 - Githubissues

dimpase commented 1 year ago

this will fix #8

dimpase commented 1 year ago

The test tst/aceds.tst hangs at

...
gap> stats:=ACEStats( i );; Unbind(stats.cputime); stats;
rec( activecosets := 80, cputimeUnits := "10^-2 seconds", 
  index := 80, maxcosets := 123, totcosets := 187 )
gap> ACEDeleteSubgroupGenerators( i, [ t ] );

hitting ^C gives

^CError, user interrupt in
  moreOfline := ReadLine( iostream )
 ; at /mnt/opt/gap/lib/streams.gi:1478 called from 
ReadAllLine( iostream, nofail, function ( line )
      return 0 < Length( line ) and line[Length( line )] = '\n';
  end ) at /mnt/opt/gap/lib/streams.gi:1498 called from
ReadAllLine( iostream, true 
 ) at /mnt/opt/gap/pkg/ace/gap/streams.gi:40 called from
readline( iostream ) at /mnt/opt/gap/pkg/ace/gap/streams.gi:61 called from
FLUSH_ACE_STREAM_UNTIL( datarec.stream, 3, 3, ACE_READ_NEXT_LINE
  , function ( line )
      return IsMatchingSublist( line, "  #--" );
  end ); at /mnt/opt/gap/pkg/ace/gap/interact.gi:1504 called from
ACE_ARGS( ioIndexAndOptval[1], "sgens" 
 ) at /mnt/opt/gap/pkg/ace/gap/interact.gi:2538 called from
...  at *stdin*:9
you can 'return;'
brk>

Probably another ACE output parsing bug.

dimpase commented 1 year ago

todo:

[x] get rid of statistics/stats ACE binary options - they are removed in ACE4 - in particular ACEDumpStatistics
[x] fix ACEBinaryVersion - it breaks due to parsing for host in gap/interact.gi:1612
[x] sort out and remove parsing for host info in gap/options.gi: options := line -> IsMatchingSublist(line, " host info")
[x] remove statistics/stats and host stuff from the docs
[x] remove ACEDumpVariables, as dump ACE command has been removed (cf. src/Diary)

codecov[bot] commented 1 year ago

Codecov Report

Attention: Patch coverage is 50.00000% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 38.46%. Comparing base (ac8eb7b) to head (0a1dc19).

:exclamation: Current head 0a1dc19 differs from pull request most recent head 720621a

Please upload reports for the commit 720621a to get more accurate results.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #36 +/- ## ========================================== - Coverage 38.82% 38.46% -0.37% ========================================== Files 10 10 Lines 2761 2704 -57 ========================================== - Hits 1072 1040 -32 + Misses 1689 1664 -25 ``` | [Files](https://app.codecov.io/gh/gap-packages/ace/pull/36?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gap-packages) | Coverage Δ | | |---|---|---| | [gap/interact.gd](https://app.codecov.io/gh/gap-packages/ace/pull/36?src=pr&el=tree&filepath=gap%2Finteract.gd&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gap-packages#diff-Z2FwL2ludGVyYWN0Lmdk) | `100.00% <ø> (ø)` | | | [gap/options.gi](https://app.codecov.io/gh/gap-packages/ace/pull/36?src=pr&el=tree&filepath=gap%2Foptions.gi&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gap-packages#diff-Z2FwL29wdGlvbnMuZ2k=) | `55.95% <100.00%> (-1.09%)` | :arrow_down: | | [gap/general.gi](https://app.codecov.io/gh/gap-packages/ace/pull/36?src=pr&el=tree&filepath=gap%2Fgeneral.gi&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gap-packages#diff-Z2FwL2dlbmVyYWwuZ2k=) | `30.40% <50.00%> (-0.85%)` | :arrow_down: | | [gap/interact.gi](https://app.codecov.io/gh/gap-packages/ace/pull/36?src=pr&el=tree&filepath=gap%2Finteract.gi&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gap-packages#diff-Z2FwL2ludGVyYWN0Lmdp) | `23.91% <25.00%> (+0.02%)` | :arrow_up: |

dimpase commented 1 year ago

All what ACEBinaryVersion is printing with ACE4 is

gap> ACEBinaryVersion();
#I  No interactive ACE sessions are currently active
#I  ACE Binary Version: 4.100
#I  ACE 4.100                       Mon Mar 13 16:45:40 2023
#I  ========================================================
"4.100"

and it breaks if there was some activity enumerating cosets before - due to a parsing error of the output. I think it makes little sense to keep it, I'll remove it.

dimpase commented 1 year ago

Removed ACE command in 4.1 to take care of:

[x] ai and ao (alter input/output)
[x] ho - hole limit
[x] par[ameters] - old option (ignored)
[x] restart - old option (ignored)
[x] pd mo[de] / pmod[e] : [0/1..3] ; becomes (?) pd[efinitions] : [0/1] ;
[x] dump, stats

dimpase commented 1 year ago

Changed in ACE4 commands:

[x] pr, as confirmed by Colin Ramsey 2023-03-13:

takes two arguments, the beginning and ending
coset.  If the arguments are negative they mean (respectively) add
order/representative columns to the table and include coincident coset rows. 
To compact and standardise the coset table you need to invoke st; before pr;,
as you note.

So our change (to always call st before pr) makes perfect sense.

dimpase commented 1 year ago

@fingolfin @hulpke - please have a look.

dimpase commented 1 year ago

Hmm, one can't use xetex to build pdf docs? Why is that - it's coming from GAP's own actions setup...

dimpase commented 1 year ago

Proposed to fix this in https://github.com/gap-actions/build-pkg-docs/pull/19

dimpase commented 1 year ago

in 6f269cc I changed the CI to use my fork of build-pkg-docs GH action, which does install xetex and metapost.

As you see, all is good.

dimpase commented 1 year ago

Ideally, my PR https://github.com/gap-actions/build-pkg-docs/pull/19 ought to be merged first, tag v1 adjusted, then the line https://github.com/gap-packages/ace/blob/6f269cc8b8f35dfc9daee7ae7ee5e8c656649e54/.github/workflows/CI.yml#L64 changed in commit 6f269cc here can and should be undone.

dimpase commented 7 months ago

As you didn't like https://github.com/gap-actions/build-pkg-docs/pull/19, please tell how you see this going forward. Do you want ACE to have its own special action?

dimpase commented 7 months ago

Anyhow, this is working, the only issue here to resolve is that the docbuld action lives in my GitHub fork.

fingolfin commented 7 months ago

No need for a special action. You can just insert a step which installs the "missing" packages. E.g. https://github.com/gap-packages/polymaking/blob/master/.github/workflows/CI.yml does this to install polymake

dimpase commented 7 months ago

No need for a special action. You can just insert a step which installs the "missing" packages. E.g. https://github.com/gap-packages/polymaking/blob/master/.github/workflows/CI.yml does this to install polymake

I've modified https://github.com/dimpase/build-pkg-docs/blob/patch-1/action.yml so that installing xetex and metapost is optional, and doesn't happen by default. If this is too special to your taste, I can probably change it so that optionally one can specify arbitrary extra Debian packages to install, not a fixed list.

dimpase commented 7 months ago

OK, now d8382f5 does what you suggest.

(I feel like yaml is slowly swapping in my brain to take place of some other language in cache 🥲 )

dimpase commented 7 months ago

some commits can certainly be squashed if needed

dimpase commented 7 months ago

@hulpke

hulpke commented 7 months ago

@hulpke

? Yes ? What ?

dimpase commented 7 months ago

Please review if possible

dimpase commented 5 months ago

I'm asking Leonard @lhsoicher to test this.

dimpase commented 5 months ago

@gregg0 - are you still following this?

gregg0 commented 5 months ago

Dear Dima,

I don’t understand what’s being done. Is there something that explains it?

Regards, Greg

Get Outlook for iOShttps://aka.ms/o0ukef

From: Dima Pasechnik @.> Sent: Tuesday, February 6, 2024 8:16:37 PM To: gap-packages/ace @.> Cc: Gregory Gamble @.>; Mention @.> Subject: Re: [gap-packages/ace] use ACE 4.1 (PR #36)

@gregg0https://github.com/gregg0 - are you still following this?

— Reply to this email directly, view it on GitHubhttps://github.com/gap-packages/ace/pull/36#issuecomment-1929400417, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABKQEEH2JD252AFDKSVUSBTYSINKLAVCNFSM6AAAAAAVYNPIR2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRZGQYDANBRG4. You are receiving this because you were mentioned.Message ID: @.***>

dimpase commented 5 months ago

Colin released an update to ACE, and I used it to update the package, adding documentation building along the way.

gregg0 commented 5 months ago

Dear Dima,

I always found the way github did things hard to get started with, and was so grateful when Max got involved. But it looks like I really have to get my head around it now, because other things - websites - I'm involved with have moved to github.

Anyway, I trust your work ... so I won't hold you up, and I'll try to look at what your work soon.

Regards, Greg

Get Outlook for iOShttps://aka.ms/o0ukef

From: Dima Pasechnik @.> Sent: Tuesday, February 6, 2024 11:50:25 PM To: gap-packages/ace @.> Cc: Gregory Gamble @.>; Mention @.> Subject: Re: [gap-packages/ace] use ACE 4.1 (PR #36)

Colin released an update to ACE, and I used it to update the package, adding documentation building along the way.

— Reply to this email directly, view it on GitHubhttps://github.com/gap-packages/ace/pull/36#issuecomment-1930095972, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABKQEEASEVW7DRDMBUPKNKDYSJGMDAVCNFSM6AAAAAAVYNPIR2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZQGA4TKOJXGI. You are receiving this because you were mentioned.Message ID: @.***>

dimpase commented 5 months ago

Thanks for the review. I'll continue updating tomorrow. :-)

dimpase commented 5 months ago

OK, I think I've addressed everything.

And marked what I thought as clear as resolved (perhaps I should not have?)

I hope you don't mind xetex.

dimpase commented 1 month ago

well, what can I do to get this reviewed?

gap-packages / ace

use ACE 4.1 #36

Codecov Report