Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prettyprint is changing my decimals #83

Open
jonolds opened this issue Nov 3, 2024 · 14 comments
Open

prettyprint is changing my decimals #83

jonolds opened this issue Nov 3, 2024 · 14 comments
Labels
bug Something isn't working probably fixed? the issue/feature appears to be fixed/implemented, but there still may be issues

Comments

@jonolds
Copy link

jonolds commented Nov 3, 2024

For example, I'll copy and paste in:

[{"idx":0,"start":0.0,"end":1073.11,"duration":1073.11,"frameDetails":{"start_frame":430,"end_frame":46215,"start_pts":440320,"end_pts":47324160,"start_pts_time":9.98458,"end_pts_time":1073.11}},{"idx":1,"start":2143.89,"end":2175.41,"duration":31.5114,"frameDetails":{"start_frame":92760,"end_frame":93686,"start_pts":94986240,"end_pts":95934464,"start_pts_time":2153.88,"end_pts_time":2175.38}},{"idx":2,"start":2254.69,"end":2317.25,"duration":62.5537,"frameDetails":{"start_frame":97532,"end_frame":99795,"start_pts":99872768,"end_pts":102190080,"start_pts_time":2264.69,"end_pts_time":2317.24}}]

and I'll hit pretty print and get:

[ { "idx": 0, "start": 0.0, "end": 1073.1099999999999, "duration": 1073.1099999999999, "frameDetails": { "start_frame": 430, "end_frame": 46215, "start_pts": 440320, "end_pts": 47324160, "start_pts_time": 9.9845799999999993, "end_pts_time": 1073.1099999999999 } }, { "idx": 1, "start": 2143.8899999999999, "end": 2175.4099999999999, "duration": 31.511399999999998, "frameDetails": { "start_frame": 92760, "end_frame": 93686, "start_pts": 94986240, "end_pts": 95934464, "start_pts_time": 2153.8800000000001, "end_pts_time": 2175.3800000000001 } }, { "idx": 2, "start": 2254.6900000000001, "end": 2317.25, "duration": 62.553699999999999, "frameDetails": { "start_frame": 97532, "end_frame": 99795, "start_pts": 99872768, "end_pts": 102190080, "start_pts_time": 2264.6900000000001, "end_pts_time": 2317.2399999999998 } } ]

What did you do with my 1/1000000000000?! j/k, I love your plugin. This is the first beef I've ever had over years of use, and I will not be shocked at all if it's something I messed up. Thank you!

@molsonkiko
Copy link
Owner

@jonolds

This is effectively a duplicate of #81, which I foolishly closed because at the time I didn't feel like dealing with this issue even though it's obviously unacceptable.

I'm in the process of working on a fix, and I hope to have it solved by the end of the day.

molsonkiko added a commit that referenced this issue Nov 3, 2024
Need to know speed of parsing and dumping before re-implementation
to fix bug where numbers like 11.11 lose precision unnecessarily
due to stupidity of G17 formatting of numbers in C#.
See issues #81, #83.
@molsonkiko
Copy link
Owner

This commit should address this issue. If you follow my instructions for downloading an unreleased version, you can test out this fix and see if it is too your liking.

As I noted in the changelog, this fix comes at the cost of noticeably worse performance when reformatting very large files (say, several megabytes or more). I wish I could think of a way to do this with less of a hit to performance, but I don't think I'm a competent enough programmer to implement a performant and correct double-formatting algorithm from scratch.

Please let me know if you have any thoughts. If you are satisfied with this fix, I will be including it in v8.2 of JsonTools, which I aim to release in the next few weeks.

@jonolds
Copy link
Author

jonolds commented Nov 4, 2024

Awesome, I'll give it a try tonight. thank you so much!

@molsonkiko molsonkiko added probably fixed? the issue/feature appears to be fixed/implemented, but there still may be issues bug Something isn't working labels Nov 5, 2024
@molsonkiko
Copy link
Owner

JsonTools version 8.2, incorporating a fix for this issue, is now live. I will soon submit a PR to include v8.2 in the plugins manager.

@Souldrinker
Copy link

Hmm, are you sure the issue is fixed in 8.2? I noticed on a colleagues computer that when he pretty printed the same JSON file as me the output looked different. Turned out I had an ancient version (4.4) and for me this json attribute:
"RealPlannedHours":0.10
...got converted into 0.1 but 0.10000000000000001 on his (with 8.1).

I updated through the npp plugin admin to the latest available 8.1 version and then I got the same.

Then I found this repo and downloaded the 8.2 x64 release and replaced the plugin in the plugin folder (plugin admin now shows 8.2) and still I get the same incorrect response as with 8.1.

@molsonkiko
Copy link
Owner

molsonkiko commented Nov 14, 2024

@Souldrinker
That's really weird; when I reformat 0.1 with JsonTools v8.2, I get 0.1 back.

Could you do me a favor and report what you get when you parse the input in the original comment of this issue? If it has a lot of unnecessarily long representations, I'm going to need to do some soul-searching to decide how best to handle this issue. Because unfortunately this is a situation where it's literally impossible to please everyone; I've looked at several potential solutions for this issue and my current one seems the best, at least on my machine.

My current implementation for serializing a decimal number (call that d) works like this:

  1. Get a short but imprecise representation for d (the same algorithm I used back in v4.4), call that representation dstr
  2. Parse dstr, call that dstr_parsed
  3. If dstr_parsed is equal to d, return dstr
    • Since you're getting 0.10000000000000001 instead of 0.1, your machine seems to think that d is closer to 0.10000000000000001 than it is to 0.1. So I'm guessing that this issue is caused by a hardware difference between your computer and mine.
  4. Otherwise, return the most precise representation possible (this is the algorithm that was always used in v8.1).

In short, I'm trying to avoid unnecessarily losing precision when I reformat numbers, as that could cause a regression of #78, but at the same time I want to default to the old shorter representation, so that I can close this issue.

@Souldrinker
Copy link

Hmm, unfortunately looks rather similar to what the issue creator got also when I do it with 8.2 for me:
image

Going back to 8.0 fixes it:
image

@molsonkiko
Copy link
Owner

@Souldrinker

Thank you so much for testing this for me. This is bizarre and really frustrating. Since I can't replicate your problem on my machine, coming up with a reliable solution for this issue (without regressing on #78) is going to be a lot more of a PITA than I anticipated.

I'll do my best to come up with a new release that fixes this issue before Notepad++ 8.7.2 releases, so that people can get an actual fix on that NPP's version of the Plugins Manager.

@molsonkiko molsonkiko added maintainer can't replicate The core dev can't replicate this issue, making a fix difficult and removed probably fixed? the issue/feature appears to be fixed/implemented, but there still may be issues labels Nov 21, 2024
@ageseltin
Copy link

Got the same problem on Notepad++ v 8.7.4 JsonTools 8.2 when PP json's like
{"base":91.9,"plan":90.2,"current":60.4,"show":60.7,"name":""} got { "base": 91.900000000000006, "plan": 90.200000000000003, "current": 60.399999999999999, "show": 60.700000000000003, "name": "" }
or {"base":4.6,"plan":6.6,"show":6.6,"name":""} got { "base": 4.5999999999999996, "plan": 6.5999999999999996, "show": 6.5999999999999996, "name": "" }

@molsonkiko
Copy link
Owner

@ageseltin

Sorry you've had this regression. At this point it's clear that this issue continues to be widespread enough that I need to implement a more robust solution. I'll try to get a fix out soon.

@molsonkiko
Copy link
Owner

@ageseltin

Sorry, didn't mean to close this, but GitHub automatically closed this issue based on my last commit message. Could you do me a favor and check if my most recent commit fixes the issue you've described? You can download that version by following these instructions.

If anyone else reads this, I'd welcome some feedback on that commit; as I noted before this issue was already solved on my computer, so only your feedback will let me know if my most recent change helped at all.

@Souldrinker
Copy link

Yes, it seems like you finally nailed it :-)

I recently got my work computer downgraded from Win11 24H2 to a company managed 23H2, but when I tried the latest Notepad++ with version 8,1 of your plugin I still got the long odd incorrect decimals.

Not sure if my Windows locale that is and was Swedish mattered and could be a cause why you had problems reproducing the issue since we're normally using comma instead of dot as decimal separator here. However in NPP I still use English language and the decimals aren't converted from dot to comma (which they shouldn't).

Anyway I downloaded the 8.2.0.3 artifact (x64) and put it in the plugins folder and now the example all looks good again without changed decimals. Thank you :-)

@ageseltin
Copy link

@ageseltin

Sorry, didn't mean to close this, but GitHub automatically closed this issue based on my last commit message. Could you do me a favor and check if my most recent commit fixes the issue you've described? You can download that version by following these instructions.

If anyone else reads this, I'd welcome some feedback on that commit; as I noted before this issue was already solved on my computer, so only your feedback will let me know if my most recent change helped at all.

This one works well, no problems with pretty print float values. Thx

@molsonkiko molsonkiko added probably fixed? the issue/feature appears to be fixed/implemented, but there still may be issues and removed maintainer can't replicate The core dev can't replicate this issue, making a fix difficult labels Dec 16, 2024
@molsonkiko
Copy link
Owner

molsonkiko commented Dec 22, 2024

JsonTools v8.3.1 is the first official release with this fix implemented. Once a version of Notepad++ (hopefully 8.7.5 or 8.7.6) with a plugin list that includes JsonTools v8.3.1 is live for about a week with no reports of a recurrence, I'll close this issue.

NOTE: This comment previously recommended upgrading to v8.3, but v8.3.1 fixes an easily replicated crash bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working probably fixed? the issue/feature appears to be fixed/implemented, but there still may be issues
Projects
None yet
Development

No branches or pull requests

4 participants