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

Bug: Rounding issues with decimal numbers - fixed #169

Merged
merged 16 commits into from
Jul 12, 2024

Conversation

aiAdrian
Copy link
Collaborator

@aiAdrian aiAdrian commented Jul 4, 2024

Description

Issues

Checklist

  • This PR contains a description of the changes I'm making
  • I've read the Contribution Guidelines
  • I've added tests for changes or features I've introduced
  • I documented any high-level concepts I'm introducing in documentation/
  • CI is currently green and this is ready for review

@aiAdrian aiAdrian linked an issue Jul 4, 2024 that may be closed by this pull request
3 tasks
@louisgreiner
Copy link
Collaborator

Will test later but does it resolve the warnings too ?

@louisgreiner louisgreiner self-requested a review July 4, 2024 09:29
@aiAdrian
Copy link
Collaborator Author

aiAdrian commented Jul 4, 2024

Thanks, yes this final check fixes

  • the validation issue
  • the rounding issue in the pearls view
  • and the 4.33 % 2 = 0.3300000000000000007 epsilon issue (writting into database => reading from data )
    cheap fix, but the effect is good enought to start with

@louisgreiner
Copy link
Collaborator

I sadly still have issues when entering decimal numbers, i did:

  1. update travel time to 15.36 (it rounded to 15.4, good)
  2. shift the departure time on BAA to 35.7, warnings poped and round issues as well
    image

@aiAdrian
Copy link
Collaborator Author

aiAdrian commented Jul 4, 2024

I tried to reproduce the issue.

What i did:

  • I checkout main branch
  • docker compose build
  • docker compose up

The issue is not fixed -> correct the fix is not yet in the main branch - fine.

Then i checkout the branch 168-bug-rounding-issues-with-decimal-numbers and again:

  • I checkout main branch
  • docker compose build

The issue is not fixed -> bug ?? the fix is not yet in the main branch - not fine.


Then i check for the source in the browser - no updated - method can not be found (fixMachineEpsilonProblem):

  • docker compose build --no-cache
  • docker compose up

image

@emersion
Copy link
Collaborator

emersion commented Jul 4, 2024

Will try to have a look at this next week!

@aiAdrian
Copy link
Collaborator Author

aiAdrian commented Jul 8, 2024

After checking the nginx configuration, finally found this issue in the config, there was a cache setting enforcing caching main.js , ... for 1 years -> changed 1min. (60s) - this is fine and sould resolve the reported caching issue.

@emersion
Copy link
Collaborator

emersion commented Jul 8, 2024

Changing the nginx config will have performance impact in production most likely...

@aiAdrian
Copy link
Collaborator Author

aiAdrian commented Jul 8, 2024

...I understand that completely. But on the other hand, the user will ask for the latest version after a bug fix or new features and so the browser cache should be "reset" as soon as a new version is deployed. We may need to find a good time for out-of-cache. If the user refreshes more often than one time per minute, he will not get a new version - the browser is still using the latest cached version (read from disk cache). If the version on the server side does not change, HTTP will return 304 "no change", then the file will not be fully transferred, which hardly affects performance - the usual case. If there is an update on the server side, the full file must be transferred, which is correct.

@emersion
Copy link
Collaborator

emersion commented Jul 8, 2024

Angular changes the filenames in production mode (e.g. main.XXX.js where XXX is a random string) so this shouldn't be a concern I think?

@aiAdrian
Copy link
Collaborator Author

aiAdrian commented Jul 8, 2024

Sounds good, but I don't understand all the details. If I am correct, can the proposed change be merged without having a direct impact on production runtime?

@aiAdrian aiAdrian changed the title problem fixed Bug: Rounding issues with decimal numbers fixed Jul 10, 2024
@aiAdrian aiAdrian changed the title Bug: Rounding issues with decimal numbers fixed Bug: Rounding issues with decimal numbers - fixed Jul 10, 2024
@aiAdrian aiAdrian merged commit f29ce1a into main Jul 12, 2024
6 checks passed
@aiAdrian aiAdrian deleted the 168-bug-rounding-issues-with-decimal-numbers branch July 12, 2024 21:23
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.

[Bug]: Rounding issues with decimal numbers.
3 participants