Skip to content

Conversation

@gw-dev101
Copy link

addresses #25811 by replacing some of the formulas with the correct ones
(the old one only worked when both vectors had the same magnitude)

i think there is still some more work to be done on the module before it is satifactory

… functions

added documentation for some methods 
and fixed project and abs 

vec probably a lot more work
Refactor project function to handle zero vector case and calculate projection properly
Copy link
Member

@spytheman spytheman left a comment

Choose a reason for hiding this comment

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

Please add tests for the changed .project methods, to prevent future regressions.

@spytheman
Copy link
Member

You have to also run v fmt -w vlib/math (or mention the specific changed files), in order to make sure that everything is kept formatted properly.
Then commit the result and push.

@gw-dev101 gw-dev101 requested a review from spytheman November 23, 2025 12:24
@tankf33der
Copy link
Contributor

Lets wait for my test(s).

@tankf33der
Copy link
Contributor

My research.

@spytheman Yes, the current implementation is incorrect. I've checked everything and I'm not approving the merge. I have several sources to prove that his implementation is not correct.

@gw-dev101 Thanks for working on this. I appreciate it. You got confused in the details and parameters, fix the code, comments, tests and come back with new commit(s).

@gw-dev101
Copy link
Author

That's the main issue with trying to make a pr at 2am
I was awake enough to notice the old one was definitely broken
But not enough to make a clean and working fix
Hopefully my second pr will go better
My bad and sorry for wasting your time

@JalonSolov
Copy link
Contributor

Not a waste of time - it brought an issue up that not many were even thinking about.

You also don't need to make a second PR - just push modifications to this one.

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.

5 participants