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

Snapshot testing + current results snapshot #5

Merged
merged 2 commits into from
Nov 17, 2020

Conversation

kosciej
Copy link
Contributor

@kosciej kosciej commented Nov 15, 2020

Snapshot testing takes advantage of deterministic character of Cleora. Any discrepancies between original snapshot results and current ones can be then reviewed along with the code which introduced discrepancy.
Test introduced by this PR performs work for sample case and saves snapshot file.

@kosciej
Copy link
Contributor Author

kosciej commented Nov 16, 2020

It works correctly on:
stable-x86_64-apple-darwin
stable-x86_64-unknown-linux-gnu

for stable-armv7-unknown-linux-gnueabihf (ARM RaspberryPi), the order of HashMaps is different in debug string, and therefore snapshots didn't check for SparseMatrices. It works correctly for final result.

To prevent this kind of issue I can drop collecting snapshots for intermediate SnapshotMatrices or work out another solution to make snapshots stable.

}

#[derive(Debug, Default)]
pub struct InMemoryEmbeddingPersistor {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can remove pub from InMemoryEmbeddingPersistor and InMemoryEntity structs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@kosciej
Copy link
Contributor Author

kosciej commented Nov 17, 2020

Removed snapshot for sparse matrices, as it's intermediate result that contains hash table, which under some circumstances have different ordering (affects snapshot comparison).

@piobab piobab merged commit 8f12863 into BaseModelAI:master Nov 17, 2020
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