fixed nf to work with negative number #7054

Akhilbisht798 · 2024-05-16T16:20:34Z

Resolves #[Add issue number here]

Changes:

resolve #7046

Screenshots of the change:

PR Checklist

npm run lint passes
Inline documentation is included / updated
Unit tests are included / updated

welcome · 2024-05-16T16:20:36Z

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

perminder-17

Firstly, I want to apologize for the delayed review, @Akhilbisht798. Due to the maintainers being occupied with the version update, I’ll be taking over the review of your work.

I appreciate your patience and the effort you’ve put into this task. Your work is mostly accurate, but there is one issue with the rounding of numbers after the decimal point that may need to be addressed.

For example, if we use let num = nf(123.58, 1,1);, the expected output should be 123.6, as it involves rounding the numbers after the decimal point. However, in your implementation, it currently results in 123.5 without rounding off correctly. Do you think we could adjust this?

Akhilbisht798 · 2024-10-03T05:51:00Z

hey @perminder-17 I can work on this.

Akhilbisht798 · 2024-10-03T06:37:17Z

perminder-17 · 2024-10-04T20:00:33Z

Thanks, @Akhilbisht798, for your great work so far, and I apologize again for the delay—I hope you don’t mind. I’d like to invite the utility stewards, @limzykenneth, @glopzel, and @davepagurek and @nickmcintyre to share their thoughts as well.

I noticed that @limzykenneth has already opened a discussion on this issue (#7046 (comment)).

In short, should we be writing let num = nf(-123, 4); where 4 represents the number of digits to include to the left? Currently, the output produced after solving by @Akhilbisht798 is -0123, but if we count the minus sign (-) as a character, the output would be -123. The question is: should we consider the minus sign in the digit count?

From my opinion I go with not counting minus sign(-).

I’d appreciate everyone’s input on this discussion that @limzykenneth started. Also, if you have any other suggestions regarding nf(), please feel free to share. (To be honest, I haven’t used nf() much myself, so I’m a bit uncertain about it.)

limzykenneth · 2024-10-05T10:56:56Z

I think we can possibly go with not counting the minus sign, which is also the behaviour of Processing. For those who needed the numbers to line up, nfs() can solve that.

perminder-17

Everything looks good, Thanks @limzykenneth for your response..just some minor cleanups then we are good to merge. Thanks @Akhilbisht798 for your hardwork and patience :)

perminder-17 · 2024-10-05T16:26:31Z

src/utilities/string_functions.js

+    let roundedNum = Math.round(num * Math.pow(10, right));
+    roundedNum = roundedNum / Math.pow(10, right);


Can we roundooff the num by simply doing this:

Suggested change

let roundedNum = Math.round(num * Math.pow(10, right));

roundedNum = roundedNum / Math.pow(10, right);

let roundedNum = num.toFixed(right);

Let me know if it works for you @Akhilbisht798 .

Yes we can, previous commit was using it. But as you have mentioned that

For example, if we use let num = nf(123.58, 1,1);, the expected output should be 123.6, as it involves rounding the numbers after the decimal point. However, in your implementation, it currently results in 123.5 without rounding off correctly. Do you think we could adjust this?

This was not working with num.toFixed(right);. But if you want, I can change it back.

Were you also able to reproduce the same error of rounding off with num.toFixed(right);. I am currently testing and here's the build and it works well.

https://editor.p5js.org/aman12345/sketches/Pq9D8HM0h

perminder-17 · 2024-10-05T16:28:35Z

src/utilities/string_functions.js

-      return leftPart + '.' + rightPart;
-    }
+    let result = typeof rightPart === 'undefined' ?
+      leftPart : leftPart + '.' + rightPart.padEnd(right, '0');


Was there a need to write .padEnd?

Suggested change

leftPart : leftPart + '.' + rightPart.padEnd(right, '0');

leftPart : leftPart + '.' + rightPart;

can we simply add this? What you think?

So this add padding of 0 at the right decimal place if the right is more than the digit in the right.
if we doNf with right of 2 in 123.5 it will pad 123.50.
I guess it would be great. But tell me known what you think about this. I can change if it is not required.

Right, when we will use num.toFixed(right) to roundoff the numbers, I don't think we would actually need to add padding of 0 to the right.

perminder-17 · 2024-10-05T16:29:52Z

src/utilities/string_functions.js

-    }else{
-      return leftPart + '.' + rightPart;
-    }
+    let result = typeof rightPart === 'undefined' ?


Do you think @Akhilbisht798 that using ternary operator for result could be a bit complicated for the other contributors to understand in a first look. Can we use if and else block for this part only? Do you agree, or maybe you have any other thoughts? Please do let me know.

I have no problem in using if else block for this statement if that increase the look. Let me known what you think about the other changes and I will commit the changes of this with them.

just a thought...

if we want to reduce the usage of ternary operator it can be mentioned in the contributor guideline somewhere

or if we have strong opinions about it, it should be part of some lint system to give warning / error

just a thought...

if we want to reduce the usage of ternary operator it can be mentioned in the contributor guideline somewhere

or if we have strong opinions about it, it should be part of some lint system to give warning / error

It's not about reducing the use of ternary operators. In this case, I was just considering whether using if and else blocks might make the code more readable, especially when checking the type. If it's undefined, we perform one operation, and for everything else, we do something different—all in a single line. I'm open to discussion and would love to hear your thoughts on this. Otherwise ternary operator also works for me. Any feedback @Akhilbisht798 and @ashish1729 ?

let result = typeof rightPart === 'undefined' ? leftPart : leftPart + '.' + rightPart;

ashish1729 · 2024-10-06T07:37:18Z

do we need to update some test + reference ?

perminder-17 · 2024-10-06T10:56:03Z

Hmm..I realize I made an error earlier. When I reviewed the code again, I found that it wasn’t rounding the numbers as I initially thought, and the mistake was on my side. I sincerely apologize for any confusion caused. It seems your previous commit was indeed correct. I hadn’t thoroughly checked the code and was only focusing on the testing, which led me to overlook this. After testing it again now, everything appears to be working perfectly.

here's the build of your previous commit:
https://editor.p5js.org/aman12345/sketches/Pq9D8HM0h

We can revert to the previous commit, as it looks all good to me. I’m really sorry for any inconvenience caused, and then we could merge this. Really thankful for your patience and hard work @Akhilbisht798.

perminder-17 · 2024-10-06T11:04:24Z

do we need to update some test + reference ?

Yes, adding tests would be a great idea. I believe we already have a good reference sketch with a detailed explanation in the code. If @Akhilbisht798 has some time, we could also add a test for negative numbers, which is currently causing issues. You can check it here:
here:

p5.js/test/unit/utilities/string_functions.js

Line 79 in 7d393ec

test('should return correct string', function() {

we could probably add the same way as it is written here ;)

Akhilbisht798 · 2024-10-06T13:37:58Z

@perminder-17 It is not a problem. I will revert back to the previous commit and will add the test case for this one also.

Akhilbisht798 · 2024-10-06T14:40:14Z

@perminder-17 all done you can let me known.

perminder-17

Looks good @Akhilbisht798 . Thanks for your hard work:)

ashish1729 · 2024-10-06T16:03:54Z

should update the reference for negative numbers

I can see myself using this function for having a fixed-width display... but the way it is implemented negative numbers have more width due to the extra minus sign.

can be taken up as a separate task to unblock the merging of this PR

perminder-17 · 2024-10-06T16:18:28Z

should update the reference for negative numbers

Yes, we could consider adding reference for negative numbers as well. However, since the issue specifically addresses the bug 'nf() produces problematic string formatting for negative numbers,' this PR resolves that issue correctly. It might be a good idea to handle any additional improvements in a separate task.

ashish1729 · 2024-10-06T16:43:02Z

oh by reference I meant documentation, the reference page of the website ..

perminder-17 · 2024-10-06T16:46:05Z

oh by reference I meant documentation, the reference page of the website ..

Yep, I was imagining the same. Sorry... I will make the above comment more clearer. Haha😅

ashish1729 · 2024-10-06T16:51:53Z

then why is documentation considered additional improvement ? shouldn't it be in sync with any update in the code?

the PR on top has 3 checkboxes, which is included in every PR

npm run lint passes
Inline documentation is included / updated
Unit tests are included / updated

It was my understanding that documentation should be updated to check the 2nd point

perminder-17 · 2024-10-06T17:11:29Z

That's a good point. However, since the PR has been open for five months and @Akhilbisht798 has been very patient and kind in making updates and adding tests, requesting more changes one by one might not be ideal. @Akhilbisht798, if you're interested, it would be great if you could add the documentation for negative numbers, but if you're busy, I'd be happy to work on it and submit a PR. Again thanks a lot for your hard work and patience.

ashish1729 · 2024-10-06T17:13:36Z

I am also trying to learn how thing works, so thank you for explaining.

Akhilbisht798 · 2024-10-06T17:41:30Z

@perminder-17 No problem, I can add documentation changes.
but I personally think that the inline documentation for this function is fine and explain things perfectly and to just a changes for negative number won't be that big of deal and won't be changing the functionality in a big way

perminder-17 · 2024-10-06T20:54:42Z

Right, the current documentation is also very good no doubt. But as @ashish1729 said,

I can see myself using this function for having a fixed-width display... but the way it is implemented negative numbers have more width due to the extra minus sign.

Also, one more reason could be, we are not counting the minus sign, so we could also add a line of comment describing it in the code with an example sketch where it shows a negative number.

What you think?

Akhilbisht798 · 2024-10-08T04:21:29Z

ya no problem i can do that.

perminder-17 · 2024-10-08T16:00:47Z

src/utilities/string_functions.js

@@ -222,6 +222,9 @@ p5.prototype.matchAll = function(str, reg) {
 * then unused decimal places will be set to 0. For example, calling
 * `nf(123.45, 4, 3)` returns the string `'0123.450'`.
 *
+ * When the number is negative, for example, calling `nf(-123.45, 5, 2)`


Thank you for adding that. Just one last thought: could you also modify the reference example so that it appears like this?

Since we are not counting the minus sign as an extra digit, negative numbers may have greater width due to the additional minus sign. Therefore, we could show users that negative numbers occupy more space as we are not counting it.

ya no problem.

perminder-17 · 2024-10-08T17:41:35Z

currently when I run grunt yui:dev it shows me this alignment. Can we fix it? also Can we add the negitve number in the describe() function? Let me know if that works

Akhilbisht798 · 2024-10-10T15:02:05Z

hey @perminder-17 let me known

perminder-17

Looks good to me. Thanks for your work and your patience.

fixed nf to work with negative number

0e3df94

perminder-17 self-requested a review September 28, 2024 23:20

perminder-17 suggested changes Sep 29, 2024

View reviewed changes

perminder-17 self-requested a review October 4, 2024 12:40

perminder-17 reviewed Oct 5, 2024

View reviewed changes

Akhilbisht798 force-pushed the fix/nf branch from d43297d to 0e3df94 Compare October 6, 2024 13:49

added test case

972b465

perminder-17 approved these changes Oct 6, 2024

View reviewed changes

perminder-17 requested a review from davepagurek October 6, 2024 14:56

documentation changed

0905e25

perminder-17 reviewed Oct 8, 2024

View reviewed changes

refrence changes.

ce330fb

Akhilbisht798 added 2 commits October 10, 2024 20:28

nf() docs changed

8734142

changed describe function

d00865c

perminder-17 approved these changes Oct 10, 2024

View reviewed changes

		let roundedNum = Math.round(num * Math.pow(10, right));
		roundedNum = roundedNum / Math.pow(10, right);

	let roundedNum = Math.round(num * Math.pow(10, right));
	roundedNum = roundedNum / Math.pow(10, right);
	let roundedNum = num.toFixed(right);

	leftPart : leftPart + '.' + rightPart.padEnd(right, '0');
	leftPart : leftPart + '.' + rightPart;

fixed nf to work with negative number #7054

Are you sure you want to change the base?

fixed nf to work with negative number #7054

Conversation

Akhilbisht798 commented May 16, 2024

PR Checklist

welcome bot commented May 16, 2024

perminder-17 left a comment

Choose a reason for hiding this comment

Akhilbisht798 commented Oct 3, 2024

Akhilbisht798 commented Oct 3, 2024

perminder-17 commented Oct 4, 2024

limzykenneth commented Oct 5, 2024

perminder-17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akhilbisht798 Oct 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashish1729 commented Oct 6, 2024

perminder-17 commented Oct 6, 2024

perminder-17 commented Oct 6, 2024

Akhilbisht798 commented Oct 6, 2024

Akhilbisht798 commented Oct 6, 2024

perminder-17 left a comment

Choose a reason for hiding this comment

ashish1729 commented Oct 6, 2024

perminder-17 commented Oct 6, 2024 • edited Loading

ashish1729 commented Oct 6, 2024

perminder-17 commented Oct 6, 2024

ashish1729 commented Oct 6, 2024

perminder-17 commented Oct 6, 2024

ashish1729 commented Oct 6, 2024

Akhilbisht798 commented Oct 6, 2024

perminder-17 commented Oct 6, 2024

Akhilbisht798 commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perminder-17 commented Oct 8, 2024

Akhilbisht798 commented Oct 10, 2024

perminder-17 left a comment

Choose a reason for hiding this comment

Akhilbisht798 Oct 6, 2024 •

edited

Loading

perminder-17 commented Oct 6, 2024 •

edited

Loading