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

use ACE 4.1 #36

Open
wants to merge 39 commits into
base: master
Choose a base branch
from
Open

use ACE 4.1 #36

wants to merge 39 commits into from

Conversation

dimpase
Copy link
Member

@dimpase dimpase commented Mar 13, 2023

this will fix #8

@dimpase dimpase marked this pull request as draft March 13, 2023 00:18
@dimpase
Copy link
Member Author

dimpase commented Mar 13, 2023

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
Copy link
Member Author

dimpase commented Mar 13, 2023

todo:

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

statistics package was removed from ACE 4.

"options;" ACE command produces different output
in diffent state of the enumerator - perhaps, we can just
remove ACEBinaryVersion all together
@codecov
Copy link

codecov bot commented Mar 13, 2023

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).

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
@@            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 Coverage Δ
gap/interact.gd 100.00% <ø> (ø)
gap/options.gi 55.95% <100.00%> (-1.09%) ⬇️
gap/general.gi 30.40% <50.00%> (-0.85%) ⬇️
gap/interact.gi 23.91% <25.00%> (+0.02%) ⬆️

@dimpase
Copy link
Member Author

dimpase commented Mar 13, 2023

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.

reverting this commit will re-surface an ACE4-specific bug,
which makes ACEBinaryVersion hang if there is an active ACE session.
@dimpase
Copy link
Member Author

dimpase commented Mar 14, 2023

Removed ACE command in 4.1 to take care of:

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

@dimpase
Copy link
Member Author

dimpase commented Mar 14, 2023

Changed in ACE4 commands:

  • 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.

It all works, except that the option aceiputfile to write the ACE
input in an external file writes there the line

aceinputile: <file name>

- which ACE4 does not understand, but happily ignores.
So this is a minor quirk.
1) "a set of figures detailing the control flow in the
state machine that is at the core of the enumerator (see the
enum.c & enum0n.inc source files)."

2) example demonstrating new functionality of "pr;" command:
"The print table now 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;."
@dimpase
Copy link
Member Author

dimpase commented Nov 29, 2023

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

@fingolfin
Copy link
Member

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
Copy link
Member Author

dimpase commented Nov 29, 2023

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
Copy link
Member Author

dimpase commented Nov 29, 2023

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
Copy link
Member Author

dimpase commented Nov 29, 2023

some commits can certainly be squashed if needed

@dimpase
Copy link
Member Author

dimpase commented Dec 1, 2023

@hulpke

@hulpke
Copy link
Contributor

hulpke commented Dec 1, 2023

@hulpke

? Yes ? What ?

@dimpase
Copy link
Member Author

dimpase commented Dec 1, 2023

Please review if possible

Copy link
Contributor

@hulpke hulpke left a comment

Choose a reason for hiding this comment

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

I have no objection and suggest we merge these changes, but do not have time to check details.

@dimpase
Copy link
Member Author

dimpase commented Jan 30, 2024

I'm asking Leonard @lhsoicher to test this.

@dimpase
Copy link
Member Author

dimpase commented Feb 6, 2024

@gregg0 - are you still following this?

@gregg0
Copy link
Contributor

gregg0 commented Feb 6, 2024 via email

@dimpase
Copy link
Member Author

dimpase commented Feb 6, 2024

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

@gregg0
Copy link
Contributor

gregg0 commented Feb 6, 2024 via email

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.

This is a huge change, I don't have time to review it properly right now. Just some quick comments, though

PackageInfo.g Outdated Show resolved Hide resolved
PackageInfo.g Show resolved Hide resolved
PackageInfo.g Show resolved Hide resolved
PackageInfo.g Outdated Show resolved Hide resolved
.release Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
doc/ace.tex Outdated Show resolved Hide resolved
CHANGES.md Show resolved Hide resolved
# Create PDF version
pdftex manual; pdftex manual
xetex manual
Copy link
Member

Choose a reason for hiding this comment

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

Why the switch from tex and pdftex to xetex?

Copy link
Member Author

Choose a reason for hiding this comment

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

well, xetex is faster, can take unicode, and produces smaller pdf files. And more modern, too. The better question IMHO is why GAP stays on the old tech :-)

Copy link
Member Author

@dimpase dimpase Feb 8, 2024

Choose a reason for hiding this comment

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

I also spent time (using xelatex) adjusting graphics generated in the docs by metapost. And changed manual to use modern graphics stack, and not some outdated postscript-based stuff. I don't think it's necessary to re-do this on the old TeX stack.

PS. Back in Nov 2023 you didn't question the need for xetex, see #36 (comment)

doc/strategies.tex Outdated Show resolved Hide resolved
dimpase and others added 3 commits February 7, 2024 23:22
Co-authored-by: Max Horn <[email protected]>
Co-authored-by: Max Horn <[email protected]>
Co-authored-by: Max Horn <[email protected]>
@dimpase
Copy link
Member Author

dimpase commented Feb 8, 2024

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

@dimpase
Copy link
Member Author

dimpase commented Feb 8, 2024

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
Copy link
Member Author

dimpase commented May 26, 2024

well, what can I do to get this reviewed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to ACE 4.1 ?
4 participants