Skip to content

Conversation

Alokzh
Copy link
Contributor

@Alokzh Alokzh commented Mar 8, 2025

Fixes: #28

Description:

This PR fixes the implementation of Prim's algorithm to align with the API conventions.

  • Removed explicit return in the run method.
  • Added reconstructMST method to retrieve the edges.
  • Updated tests to use reconstructMST instead of relying on the return value of run.

@Ducasse Ducasse requested a review from jordanmontt March 8, 2025 16:46
@Ducasse
Copy link
Contributor

Ducasse commented Mar 8, 2025

Thanks for the PR. What is reconstructMST?

@Alokzh
Copy link
Contributor Author

Alokzh commented Mar 8, 2025

Hi @Ducasse, I think I made a mistake in naming the method here, In prim's algo MST edges are already computed and stored, so there’s no need to "reconstruct" them.

Do you think I should name it as something like getMSTEdges ( Here MST stands for Minimum Spanning Tree ) or getTreeEdges.
Please let me know , which is better or if you have some other suggestions.

@Ducasse
Copy link
Contributor

Ducasse commented Mar 8, 2025

I would call them minimumSpanningTreeEdges (get feels like a Java hack).

Now I do not get the name of the variable because does it returns a tree or edges?

prim run.
tree := prim reconstructMST collect: [ :e | e asTuple ].

@Alokzh
Copy link
Contributor Author

Alokzh commented Mar 9, 2025

Thnx @Ducasse, I have renamed the variables.

Please have a look whenever you get time.

@jordanmontt
Copy link
Member

Nice refactor :)
the name minimumSpanningTreeEdges it is way better.

Thanks for your PR @Alokih

@jordanmontt jordanmontt merged commit 9c96bb8 into pharo-ai:master Mar 9, 2025
@jordanmontt
Copy link
Member

BTW we have a failing CI (it is not your fault). I believe it can be a good exercise for you if you can make a PR to have all the CIs green. Of course, only if it interest you. What do you think @Alokih ?

@Alokzh
Copy link
Contributor Author

Alokzh commented Mar 10, 2025

For sure @jordanmontt I am working on it. Thnx

@Alokzh
Copy link
Contributor Author

Alokzh commented Mar 10, 2025

@jordanmontt I tested the workflow run on my fork repo & found 2 main issues :

  1. There was a mismatch in naming of AINetworkFlowEdge.class.st. Actual file was named AINetworkFLowEdge.class.st in repo ( Note the uppercase L in FLow ).
    So I fixed it & again ran workflow then I got another issue
    Here is the workflow run link in forked repo link

  2. CI was failing for Pharo64-9.0 specifically on the Windows runner, the issue was related to VM I think ( sqMakeMemoryNotExecutableFromTo in sqWin32SpurAlloc.c ) You could also check from above workflow link I provided.
    So what I did was removed Pharo64-9.0 from jobs & ran again
    Here is the link
    Please let me know what you think, if Pharo64-9.0 is not crictical we could remove it either only for windows or all OS.
    If it is required can you please give me some suggestions here, I am stuck at this point.

Also I have fixed this warning
Screenshot from 2025-03-10 14-22-25

@jordanmontt
Copy link
Member

Hello @Alokih

  1. Yes, you are right about the uppercase L. I don't know how it got there... Somebody must have edited the file manually.
  2. Indeed, there is a VM failure for the Pharo 9 on WINDOWS. It is working just fine on my mac. What I just did it is just to remove Pharo 9, if we have users that need it in Pharo 9 we will investigate further.

I fixed those 2 things and know we have a green CI. Thanks for your investigation!!

@Alokzh
Copy link
Contributor Author

Alokzh commented Mar 10, 2025

Glad I could help ! @jordanmontt Happy to see CI green now 🎉

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.

Not returning explicitly anything on Prim's algo

3 participants