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.

Sign up for GitHub

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

Jump to bottom

ISSUE-231 switch from xsd:double to xsd:decimal (revealed with ABETCO) #269

Merged
merged 3 commits into from
May 17, 2023
Merged

ISSUE-231 switch from xsd:double to xsd:decimal (revealed with ABETCO) #269

merged 3 commits into from
May 17, 2023

Conversation

jmkeil
Copy link
Contributor

@jmkeil jmkeil commented Sep 8, 2022

Solving #231 by replacing xsd:double with xsd:decimal for conversion values and beyond.

Please not, that this is a breaking change, as it might change datatypes used in tools to represent parsed values.

@dr-shorthair dr-shorthair self-requested a review September 8, 2022 11:15
dr-shorthair
dr-shorthair reviewed Sep 8, 2022
View reviewed changes
Copy link
Collaborator

@dr-shorthair dr-shorthair left a comment

Choose a reason for hiding this comment

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

In general I prefer xsd:decimal for serialization, and almost always specify xsd:decimal in my designs. It is more 'honest', and has arbitrary precision.

It does get a bit unwieldy for very large and very small numbers (large positive and negative exponents in scientific notation), but that is really just in a Human-oriented UI.

However, as you note, it is potentially incompatible with some existing applications, so feedback on this should be sought before proceeding.

@jmkeil jmkeil mentioned this pull request Sep 8, 2022
Closed
@jmkeil jmkeil changed the title #231 switch from xsd:double to xsd:decimal #231 switch from xsd:double to xsd:decimal (revealed with ABETCO) Sep 8, 2022
@jmkeil jmkeil changed the title #231 switch from xsd:double to xsd:decimal (revealed with ABETCO) ISSUE-231 switch from xsd:double to xsd:decimal (revealed with ABETCO) Sep 8, 2022
brandonnodnarb
brandonnodnarb requested changes Feb 15, 2023
View reviewed changes
Copy link
Member

@brandonnodnarb brandonnodnarb left a comment

Choose a reason for hiding this comment

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

in stateEnergyFlux.ttl there were errors in the form of NE-n were converted to NEn.

Example:
"9.0E-4"^^xsd:double was converted to "90000.0"^^xsd:decimal instead of "0.0009"^^xsd:decimal. Added comments for each as appropriate.

src/stateEnergyFlux.ttl Outdated Show resolved Hide resolved
src/stateEnergyFlux.ttl Outdated Show resolved Hide resolved
src/stateEnergyFlux.ttl Outdated Show resolved Hide resolved
src/stateEnergyFlux.ttl Outdated Show resolved Hide resolved
src/stateEnergyFlux.ttl Outdated Show resolved Hide resolved
src/stateEnergyFlux.ttl Outdated Show resolved Hide resolved
src/stateEnergyFlux.ttl Outdated Show resolved Hide resolved
src/stateEnergyFlux.ttl Outdated Show resolved Hide resolved
src/stateEnergyFlux.ttl Outdated Show resolved Hide resolved
src/stateEnergyFlux.ttl Outdated Show resolved Hide resolved
@brandonnodnarb
Updated stateEnergyFlux.ttl scientific notation conversion to decimal
bbbe203 
negative values in scientific notation were converted to positive values when going from xsd:double to xsd:decimal
@brandonnodnarb
Copy link
Member

it is potentially incompatible with some existing applications, so feedback on this should be sought before proceeding.

Indeed. I interpret this a 'more' correct. I advocate making the change and highlighting the change it in the v3.6 release info.

I think I corrected the negative to positive exponent issues. Can someone please verify?

jmkeil
jmkeil commented Feb 16, 2023
View reviewed changes
Copy link
Contributor Author

@jmkeil jmkeil left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, something went wrong during the automated replacement in src/stateEnergyFlux.ttl. Looks good now.

@brandonnodnarb brandonnodnarb linked an issue Feb 20, 2023 that may be closed by this pull request
Use of xsd:decimal instead of xsd:double for conversion factors #231
Closed
rduerr
rduerr reviewed Mar 15, 2023
View reviewed changes
src/relaMath.ttl Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good

rduerr
rduerr previously approved these changes Mar 15, 2023
View reviewed changes
Copy link
Contributor

@rduerr rduerr left a comment

Choose a reason for hiding this comment

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

Looks good!

@rduerr
Copy link
Contributor

rduerr commented Mar 15, 2023

@jmkeil Can you please fix the conflicts in src/reprSciUnits.ttl. I was concerned about changing the abbreviation for Siemens from S to G, in particular...

brandonnodnarb
brandonnodnarb previously approved these changes Apr 11, 2023
View reviewed changes
r0sek
r0sek previously approved these changes May 17, 2023
View reviewed changes
@r0sek
Copy link
Collaborator

r0sek commented May 17, 2023

Approved

@r0sek r0sek closed this May 17, 2023
@brandonnodnarb
Copy link
Member

closed on accident.

@brandonnodnarb brandonnodnarb dismissed stale reviews from r0sek, rduerr, and themself via 5925c47 May 17, 2023 20:53
brandonnodnarb
brandonnodnarb approved these changes May 17, 2023
View reviewed changes
Copy link
Member

@brandonnodnarb brandonnodnarb left a comment

Choose a reason for hiding this comment

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

FIxed symbol/abbreviation mismatch

@brandonnodnarb brandonnodnarb merged commit 9586d8e into ESIPFed:master May 17, 2023
@jmkeil
Copy link
Contributor Author

jmkeil commented May 22, 2023

@jmkeil Can you please fix the conflicts in src/reprSciUnits.ttl. I was concerned about changing the abbreviation for Siemens from S to G, in particular...

The wrong symbols did not origin from my PR, but from this PRs parent version (69655a7). The symbols have been fixed in my PR #271, which was merged after this PRs parent commit. Preserving the correct symbols is a matter of propper merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dr-shorthair dr-shorthair dr-shorthair left review comments

@brandonnodnarb brandonnodnarb brandonnodnarb approved these changes

@r0sek r0sek r0sek approved these changes

@rduerr rduerr rduerr left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use of xsd:decimal instead of xsd:double for conversion factors
5 participants
@jmkeil @brandonnodnarb @rduerr @r0sek @dr-shorthair