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

Test comparing log-signature with iisignature still doesn't pass. #55

Merged
merged 4 commits into from
Mar 27, 2024

Conversation

crispitagorico
Copy link
Contributor

Replaced signatory with iisignature for tests. Test comparing log-signature with iisignature still doesn't pass.

@anh-tong
Copy link
Owner

anh-tong commented Mar 11, 2024

Hi Cris,

Thank you for this pull request. I agree that test with iisignature can reduce the CI dependency installation time.

Regarding some of the test with log signatures, signax currently follows Lyndon basis while iisignature mainly uses Hall basis. Using the same basis can correctly match between the two outputs.

Once the test is fixed, the PR can be ready to merge.

@anh-tong anh-tong self-requested a review March 11, 2024 10:56
@crispitagorico
Copy link
Contributor Author

Hi Anh,

Thanks for your reply. The Lyndon basis IS a Hall basis, and it is the default choice of basis for the FLA in iisignature. I suspect the issue here is that, as in Patrick's signatory, you are lacking a final linear map to obtain the correct log-signature in the Lyndon basis. See sections 4.3 and A.2.3 in https://arxiv.org/pdf/2001.00706.pdf.

@anh-tong
Copy link
Owner

Thanks for clarifying the point about the basis. Like you said, there are different modes (words, brackets, expand) for logsignature outputs in signatory (see signatory doc).
And the default (mode words) output of signatory requires a linear map to correct logsignature (signatory test file )

Including different options like this can be good idea for the next upgrade of signax.

For now, the test will just focus on the mode words.

@crispitagorico
Copy link
Contributor Author

For the log-signature to be mathematically correct, the default option should be mode=brackets imo. Computing the log-signature in the correct basis is important for scientific ML applications, RDE solvers etc. For "black-box" deep learnng it might not be necessary. It suffices to compose the current output with a final explicit linear map. I'll have a go at it soon.

@anh-tong
Copy link
Owner

Sorry for taking so long. I have issued #56 and will try to address this later. The test for log signatures (and some problems in CI) will be fixed in next PR that I'm working on now.

Thank you for this PR.

@anh-tong anh-tong merged commit 539e160 into anh-tong:main Mar 27, 2024
3 of 5 checks passed
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.

2 participants