Skip to content

Conversation

@pankaj-bind
Copy link
Contributor

Hey, the AIEuclideanDistanceTest class in the AI-EditDistances-Tests package could use a few more tests to make sure the Euclidean Distance metric is rock-solid:

Edge Cases: We’re missing tests to check what happens with empty collections (should give 0) or identical collections (should also give 0). These are super important to make sure the code handles the basics smoothly.
Invalid Inputs: There’s no test to catch cases where collections have different lengths (should throw an error) or include non-numeric stuff like strings (should also throw an error).

@Alokzh
Copy link
Contributor

Alokzh commented May 5, 2025

Hi @pankaj-bind Just a doubt I mean are you pushing these changes using Iceberg because I think if any test method has the protocol as *AI-EditDistances-Tests then after pushing the changes using Iceberg it would automatically add some methods to AIEuclideanDistanceTest.extension.st

Even I am not 100% sure about this , please correct me if I am wrong

@pankaj-bind
Copy link
Contributor Author

@Alokzh AIEuclideanDistanceTest is defined in the AI-EditDistances-Tests package, adding new test methods (with protocol *AI-EditDistances-Tests) will update AIEuclideanDistanceTest.class.st, not AIEuclideanDistanceTest.extension.st. Iceberg won’t touch the extension file unless we’re extending a class from another package.

@Alokzh
Copy link
Contributor

Alokzh commented May 5, 2025

I am a bit confused regarding this from long time now , maybe I need to learn more how this works now & how it used to work in past

BTW Thnx for the explanation
Also can I ask one more question why using *AI-EditDistances-Tests as protocol for the tests you wrote, I mean why not regular protocol such as tests ?

@pankaj-bind
Copy link
Contributor Author

Hey @Alokzh, no worries, it can be confusing! I used *AI-EditDistances-Tests as the protocol to keep test methods organized within the AI-EditDistances-Tests package, following Pharo’s convention for categorizing tests. Using a plain tests protocol would work but might mix things up with other packages. It’s just a way to stay tidy and consistent with the project’s structure.

Also, you’re doing great, and you’re on the right track—your contributions are quite impressive!

@Alokzh
Copy link
Contributor

Alokzh commented May 6, 2025

Hey @Alokzh, no worries, it can be confusing! I used *AI-EditDistances-Tests as the protocol to keep test methods organized within the AI-EditDistances-Tests package, following Pharo’s convention for categorizing tests. Using a plain tests protocol would work but might mix things up with other packages. It’s just a way to stay tidy and consistent with the project’s structure.

Ok got it

Also, you’re doing great, and you’re on the right track—your contributions are quite impressive!

Thnx a lot !

@pankaj-bind
Copy link
Contributor Author

Hey! @jordanmontt, please review this PR

@jordanmontt
Copy link
Member

Great

@jordanmontt jordanmontt merged commit ddc4459 into pharo-ai:master May 7, 2025
1 of 6 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.

3 participants