Skip to content

CI fixed#47

Merged
jordanmontt merged 7 commits intopharo-ai:masterfrom
pankaj-bind:fixed-CI
May 5, 2025
Merged

CI fixed#47
jordanmontt merged 7 commits intopharo-ai:masterfrom
pankaj-bind:fixed-CI

Conversation

@pankaj-bind
Copy link
Copy Markdown
Contributor

issue #45 resolved
to see the CI passing all test first merge this pharo-containers/Containers-Array2D#32
as current Containers-Array2D lacks some methods.
image

@pankaj-bind
Copy link
Copy Markdown
Contributor Author

pankaj-bind commented May 2, 2025

@jordanmontt ignore these failing CI once you merge my PR opened in Containers-Array2D, it will automatically get resolved
I have tested in my system offline over Pharo version 9, 10, 11,12,13

image
image
image
image
image

columns: firstString size + 1.
| distanceMatrix |
distanceMatrix := CTArray2D
extent: (firstString size + 1) @ (secondString size + 1).
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would say to instantiate the array 2d with the method #rows:columns: instead, for clarity

self fillFirstTwoRowsAndColumnsWith: firstString and: secondString
]
distanceMatrix := CTArray2D
extent: (secondString size + 2) @ (firstString size + 2).
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same here

I would say to instantiate the array 2d with the method #rows:columns: instead, for clarity

with: [ spec repository: 'github://pharo-containers/Containers-Array2D/src' ].
"Packages"
spec
package: 'AI-EditDistances' with: [ spec requires: #( 'AIExternalVectorMatrix' ) ];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you need to add the dependency to ContainersArray2D here, just as external vector matrix

@jordanmontt
Copy link
Copy Markdown
Member

Nice, let's wait tp have a green ci to integrate this. We need to resolve the previous pr first

@pankaj-bind
Copy link
Copy Markdown
Contributor Author

@jordanmontt all issues have been resolved

@Alokxk
Copy link
Copy Markdown
Contributor

Alokxk commented May 3, 2025

@pankaj-bind I think there is a problem with the repository url format for the ContainersArray2D in baseline

The correct url should be github://pharo-containers/Containers-Array2D/src instead of github.com/pharo-containers/Containers-Array2D/src

@Alokxk
Copy link
Copy Markdown
Contributor

Alokxk commented May 3, 2025

@pankaj-bind Now I think we need to resolve the previous PRs Implementation to make CI green
Just wanted to ask are you working on it ?
If yes then good or else can I try if you are not working on it ?

@pankaj-bind
Copy link
Copy Markdown
Contributor Author

pankaj-bind commented May 3, 2025

@Alokzh it already resolved, I have added some new methods in Array2D and created a PR for that once that get merged, here CI will become green.

@Alokxk
Copy link
Copy Markdown
Contributor

Alokxk commented May 3, 2025

Ah alright, I didn't see that
Thnx for clarifying

@jordanmontt jordanmontt merged commit bc4491e into pharo-ai:master May 5, 2025
0 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