Added the use of variable names other than 'x' #48

mocenigo · 2025-04-05T17:18:02Z

As the name says, I added the use of variable names other than 'x'.
Any letter except 'c' and 'p' can be used.
I am also skipping underscores so a(2), a2, and a_2 should be equivalent,

Clauses that begin like q_0 = a_xb_x should be equivalent to q_0 + a_xb_x , unless I have misunderstood the syntax.

Finally, I outout a ".resolve" file that explains the correspondences between variables and numbers.

Note that at the moment I am adding 2 to the variable count because I am leaving "0 signifies 0" and "1 signifies 1" that the Courtois converter generated. If these are actually not used/needed, I can amend it.

Added support for single letter variables names different from x

msoos · 2025-04-05T20:24:50Z

Thanks, looks nice! I'll review tomorrow, but it looks good to me :)

msoos · 2025-04-06T12:40:38Z

Hi,

I have now made the GitHub actions work, and build a binary for linux amd64 and arm64, and mac arm64. It now also runs a test suite. You can find the tests under tests/anf-tests -- they specify what needs to run and what the output should mostly look like. It's a very simple system. Please check it out and adjust your PR to match them. Sorry for the system being in such a sorry state before. With this change, I think your PR will also be a lot more impactful, as Bosphorus will finally be available for different architectures, etc.

Once your PR is merged, I will create a new release, and attach the auto-generated binaries, e.g. the ones at the bottom of this page here:

https://github.com/meelgroup/bosphorus/actions/runs/14292531540

but of course after your PR is merged.

Sorry for this delay. I worked on this for a few hours, so hopefully it'll now be a lot easier to release things and to build them for different target architectures.

Mate

mocenigo · 2025-04-06T13:32:46Z

HI, I am not sure what you mean by "Please check it out and adjust your PR to match them" (them = the tests).

msoos · 2025-04-06T15:21:54Z

Hi! I just meant that I saw you added some test cases -- I was just trying to ask you to adjust your test cases to the pattern that's in that directory. I think it should be easy, but let me know if you have any trouble! Thank you a lot!

mocenigo · 2025-04-06T18:05:51Z

Hi! I just meant that I saw you added some test cases -- I was just trying to ask you to adjust your test cases to the pattern that's in that directory. I think it should be easy, but let me know if you have any trouble! Thank you a lot!

Oh, interesting, I am not sure how those ended up there. I may have mixed up things.
They seem to be all tests that were in this repo previously (at leat last year). I however pulled the latest before branching for my changes, so they must be yours.

mocenigo · 2025-04-06T18:14:38Z

bosphorus --anfread substitute_carefully.anf --cnfwrite substitute_carefully.cnf
crashes:
libc++abi: terminating due to uncaught exception of type polybori::PBoRiError: Variable index out of bounds.
(but also the master from your repo, I think)

FOUND the issue in ANF::readFileForMaxVar — need to handle ()_ better.

test.anf

msoos · 2025-04-06T23:11:33Z

test.cnf

Same as with test.anf

msoos · 2025-04-06T23:11:48Z

testcmp.anf

Same as with test.anf

msoos · 2025-04-06T23:11:54Z

tests/el.cnf

Same as with test.anf

.gitignore

src/anf.cpp

msoos · 2025-04-07T09:37:13Z

Let’s not make it work for special characters :) Instead, let’s emit an error of that happens.

…

On Mon 7. Apr 2025 at 11:35, mocenigo ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/anf.cpp <#48 (comment)>: > } size_t ANF::readFile(const std::string& filename) { // Read in the file line by line vector<std::string> text_file; + size_t maxes[26] = {0}; I was assuming that only a-z and A-Z can be used, but in this case I am also making assumptions on their encoding (ASCII). Maybe using tolower and a std::is better, so people can also use ö or δ. Why not? — Reply to this email directly, view it on GitHub <#48 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKF4ON7IZKYFFS7JH5BAS32YJBHNAVCNFSM6AAAAAB2QWYMCKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDONBWGE4DCNRZG4> . You are receiving this because you commented.Message ID: ***@***.***>

mocenigo · 2025-04-07T09:59:12Z

Let’s not make it work for special characters :) Instead, let’s emit an error of that happens.

Anyway, since there are then 26 characters (up to lower case/upper case equivalence) I would keep the array of 26 values.
note that I am keeping counts of the variables and already adding +1 there, so I removed this addition from bosphorus.cpp .

Omitting "c" and "p" means that there are only 24 valid characters, I am not sure that allowing "C" and "P" are good ideas, so I am not allowing them. This means that their maxes counters will remain zero.

mocenigo · 2025-04-09T20:48:13Z

HI @msoos it is not clear to me what I should change. Since we only use latin alphabet letters for the variables, the 26-element array for the counts is sufficient, I have decuded to keep it and I have added a comment. I ran some tests, and added a test using a "ü" — what else should I do?

msoos

HI,

Thanks, looking a lot better! Can you please delete the CNF files from the test directory? Also, all .anf files need to have RUN and CHECK directives that you are adding. I also don't understand the .resolve file. This sounds like maybe some old remnant? Maybe we just need to rebase this PR, let me try doing that.

msoos · 2025-04-10T20:55:38Z

src/anf.cpp

-    size_t maxVar = 0;
+
+    // Only 26 because we allow only a-z and A-Z as variable names (case insensitive) (note we skip c, p)
+    size_t maxes[26]  = {0};


Hi! Can we rename this variable? "maxes" is something I don't really understand, doesn't seem intuitive

msoos · 2025-04-10T20:56:32Z

src/anf.cpp

+                int index = (varLetter-'a'); // variables are zero based
+                //printf("%c %zu ", varLetter, var);
+                if (maxes[index] <= var) {
+                    maxes[index] = var+1;


Maybe maxVarForLetter or something?

msoos · 2025-04-10T20:57:44Z

src/anf.cpp

 {
    // Read in the file line by line
    vector<std::string> text_file;
+    size_t maxes[26]  = {0};


Wait, does that zero out maxes? I am still very unhappy about this being a statically sized array. Can we make it into a

vector<uint32_t> maxVarForLetter(26,0)

Please? To me that's a lot easier to deal with. The performance hit is basically zero, but it's a lot easier to deal with.

msoos · 2025-04-10T20:58:51Z

src/anf.cpp

+    size_t maxes[26]  = {0};

-    size_t maxVar = 0;
+    size_t currentVar = 2;


I don't understand why it starts at 2? Is there a reason?

Sorry for the long absence, I have had some work emergencies and also a family one. And after that a few days with a fever. The addition of 2 was due to a program that needs 0 and 1 for 0 and 1 instead of variables, but we should just renumber variables progressively.

Additionally, the "bad_char.anf.resolve" files where just the mappings from new variables to the old names, since some programs need that to reconstruct the original problem. I will fix them instead and start from 0.

In the future I might modify this to accept variables made of letters + (optional)"_" + numbers, but at the moment a single letter different from c should be fine.

msoos · 2025-04-10T21:02:00Z

Can't seem to be able to rebase to remove those ANF and CNF and RESOLVE files. Can you please remove them from your PR?

mocenigo · 2025-04-11T23:31:44Z

Sorry, I did not have time to work on this today. I will try and find time tomorrow.

…

On 10. Apr 2025, at 23:02, Mate Soos ***@***.***> wrote: msoos left a comment (meelgroup/bosphorus#48) Can't seem to be able to rebase to remove those ANF and CNF and RESOLVE files. Can you please remove them from your PR? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread. <#48 (comment)> <https://github.com/notifications/unsubscribe-auth/ABFK7QOHW57I6VEP4GUUXLD2Y3L57AVCNFSM6AAAAAB2QWYMCKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOJVGE2TSOJQGM> msoos left a comment (meelgroup/bosphorus#48) <#48 (comment)> Can't seem to be able to rebase to remove those ANF and CNF and RESOLVE files. Can you please remove them from your PR? — Reply to this email directly, view it on GitHub <#48 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFK7QOHW57I6VEP4GUUXLD2Y3L57AVCNFSM6AAAAAB2QWYMCKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOJVGE2TSOJQGM>. You are receiving this because you authored the thread.

mocenigo · 2025-05-04T13:25:45Z

I have now made the GitHub actions work, and build a binary for linux amd64 and arm64, and mac arm64. It now also runs a test suite. You can find the tests under tests/anf-tests -- they specify what needs to run and what the output should mostly look like. It's a very simple system. Please check it out and adjust your PR to match them. Sorry for the system being in such a sorry state before. With this change, I think your PR will also be a lot more impactful, as Bosphorus will finally be available for different architectures, etc.

Sadly, I do not understand how the test system works. Anyway, I changed the name of the "maxes" array and used the vector<uint32_t> you suggested instead. I changed the ending of the "resolve" files to mapping, so that one can used them to map back to the original variables.

msoos and others added 3 commits October 25, 2024 12:19

Fixing up to new cmake

ae915ce

ignore

1a0b48c

Variables other than x

fb3096e

Added support for single letter variables names different from x

msoos force-pushed the master branch from ae915ce to 6be0ad4 Compare April 6, 2025 10:24

Merge branch 'master' into variable-letters

067cb92

mocenigo added 2 commits April 6, 2025 20:38

Fixed parsing bug in ANF::readFileForMaxVar

2111794

Update anf.cpp

79fda21

msoos reviewed Apr 6, 2025

View reviewed changes

test.anf Outdated Show resolved Hide resolved

msoos reviewed Apr 6, 2025

View reviewed changes

test.cnf Outdated

Copy link

Collaborator

msoos Apr 6, 2025

Choose a reason for hiding this comment

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

Same as with test.anf

msoos requested changes Apr 6, 2025

View reviewed changes

Improved upper/lower support and detection of bad chars

9ca6435

mocenigo requested a review from msoos April 7, 2025 10:19

mocenigo added 4 commits April 7, 2025 21:10

Update anf.cpp

2fb8552

Update test.anf

18c2fb3

Update anf.cpp

01d31c9

Update anf.cpp

b6cc97b

msoos requested changes Apr 10, 2025

View reviewed changes

cleanup

e14a540

Added the use of variable names other than 'x' #48

Are you sure you want to change the base?

Added the use of variable names other than 'x' #48

Uh oh!

Conversation

mocenigo commented Apr 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msoos commented Apr 5, 2025

Uh oh!

msoos commented Apr 6, 2025

Uh oh!

mocenigo commented Apr 6, 2025

Uh oh!

msoos commented Apr 6, 2025

Uh oh!

mocenigo commented Apr 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mocenigo commented Apr 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

msoos commented Apr 7, 2025 via email

Uh oh!

mocenigo commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mocenigo commented Apr 9, 2025

Uh oh!

msoos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msoos commented Apr 10, 2025

Uh oh!

mocenigo commented Apr 11, 2025 via email

Uh oh!

mocenigo commented May 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mocenigo commented Apr 5, 2025 •

edited

Loading

mocenigo commented Apr 6, 2025 •

edited

Loading

mocenigo commented Apr 6, 2025 •

edited

Loading

mocenigo commented Apr 7, 2025 •

edited

Loading