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

pass through gap data (target/met) values with rounding to 4 decimal digits [MRXN23-601] #1667

Conversation

hotzevzl
Copy link
Member

@hotzevzl hotzevzl commented Mar 15, 2024

@hotzevzl hotzevzl self-assigned this Mar 15, 2024
Copy link

vercel bot commented Mar 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
marxan ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 20, 2024 1:26pm

@hotzevzl
Copy link
Member Author

@andresgnlez thank you for reviewing the tiny changes in this PR. I have checked the way this works and it all looks ok - one thing I should mention is that because of reasons, the ORM gives us these values as numbers-in-a-string now (basically, because the possible precision of the db types for these values may not "fit" within JS numeric types) -- so while this "works", I wanted to make you aware of this in case you can think of any possible caveats/edge cases with this.

Or maybe I should "just" re-cast these numbers-in-a-string to proper JS numbers in the API, and avoid getting numbers in weird formats to API clients to start with... I'd do this next week

@agnlez
Copy link
Member

agnlez commented Mar 18, 2024

@hotzevzl happy to test this once is available in m23 instance, AFAIK the change is exclusive of the gap-data endpoint, isn't it? Let me know if you plan to extend this change through the codebase, if so, I might need to take a deeper look as we might be using Number methods straightforward in some cases or just the fact of having arithmetic operations between strings and number (they should work, but in JS world anything could happen 😬 ) scares me.

@hotzevzl
Copy link
Member Author

@hotzevzl happy to test this once is available in m23 instance, AFAIK the change is exclusive of the gap-data endpoint, isn't it? Let me know if you plan to extend this change through the codebase, if so, I might need to take a deeper look as we might be using Number methods straightforward in some cases or just the fact of having arithmetic operations between strings and number (they should work, but in JS world anything could happen 😬 ) scares me

@andresgnlez I have just added a cast back from number-as-string to actual JS number via parseFloat() - so this should not cause unexpected new oddities. The cast to float should be fairly safe as we are already rounding the values to 4 decimal digits in the SQL view, so there's no chance we get values from db that cannot be held in a JS number without rounding. We should never get nulls or other funky (Infinity and beyond) values from the db either, but I'm handling these cases anyway.

@hotzevzl hotzevzl merged commit 481218f into develop May 9, 2024
61 checks passed
@hotzevzl hotzevzl deleted the spike/geoprocessing/MRXN23-601_round-gap-data-to-decimal-digits branch May 9, 2024 12:00
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.

2 participants