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

Input Data Constraints Table | Significant Digits and Units #868

Closed
4 tasks done
samm82 opened this issue Jul 12, 2018 · 12 comments · Fixed by #1250
Closed
4 tasks done

Input Data Constraints Table | Significant Digits and Units #868

samm82 opened this issue Jul 12, 2018 · 12 comments · Fixed by #1250
Assignees

Comments

@samm82
Copy link
Collaborator

samm82 commented Jul 12, 2018

Significant Digits

Units

(Related to discussion in #824) In Drasil, a and b are given in metres, and in millimetres in caseStudies.

sig figs and units

@smiths
Copy link
Collaborator

smiths commented Jul 12, 2018

Yes, propagating the number of significant digits sounds fine. Since you brought it up, we should actually change the Drasil version to 10% for the uncertainty. 10.0% implies a precision that we don't have.

For TNT, I agree that 1.0 makes sense for both options.

@elwazana is currently working on fixing the units so that they are consistent. (Discussed in #824.) This means first checking the units in Drasil and then propagating these corrections to the case study version. In all likelihood the mm units in the case study version will be changed to m.

@smiths smiths assigned samm82 and unassigned smiths Jul 12, 2018
samm82 added a commit to smiths/caseStudies that referenced this issue Jul 13, 2018
samm82 added a commit that referenced this issue Jul 13, 2018
@samm82
Copy link
Collaborator Author

samm82 commented May 1, 2019

My current fix to the 10.0% vs 10% discrepancy rounds the uncertainty to an integer if it is equal (approximately because of floats) and leaves it as a float otherwise. Is this desired @JacquesCarette @smiths? This also propagates through all examples and assumes the precision of all 10% uncertainties to be the same.

EDIT: If so, since we aren't maintaining caseStudies anymore, this issue can be closed with the merge of glassImprovements.

@smiths
Copy link
Collaborator

smiths commented May 2, 2019

@samm82, I don't love the solution of casting a float to an integer. It would be better if we rounded the float to zero decimal places, but still considered it a float. I could see a day where we keep track of the number of significant digits and propagate that through calculations, but that is down the road.

The rounding should happen at display time. We shouldn't actually change the value, just how it is displayed.

@samm82
Copy link
Collaborator Author

samm82 commented May 2, 2019

I found a better solution in (2cb16c6) that involves using the Decimal package - would that be acceptable if I added it to the build-depends in the cabal file? Decimals are floats with a set precision, and I think function perfectly for this purpose, especially since the float is converted straight to a Sentence in the found function. However, the current solution involves setting the precision to 0 if the uncertainty is 10% and setting it to 1 otherwise; this could be automated later.

@smiths
Copy link
Collaborator

smiths commented May 2, 2019

The Decimal package sounds good, although @JacquesCarette should weigh in on the use of external packages. I like the idea that we can restrict the precision, but basing that on the value of the uncertainty seems like a hack we don't want to do. @JacquesCarette will likely know the right way to design this, but I think we need to have an option for display precision somewhere. If precision isn't specified, the default should be full precision, not 1 decimal place.

@samm82
Copy link
Collaborator Author

samm82 commented May 2, 2019

What do you think about leaving it for now, as it technically fixes this issue, closing this issue, and creating another for generalizing the precision? @smiths

@smiths
Copy link
Collaborator

smiths commented May 2, 2019

I think we should get the opinion of @JacquesCarette before proceeding with the proposed temporary solution. This might be a case where the fix is worse than the original problem. 😄 My concern is that we are introducing something that sets a bad precedent.

@samm82
Copy link
Collaborator Author

samm82 commented May 2, 2019

That's why I proposed a separate issue to deal with it, just so we can close this, and so the conversation doesn't drift from the original intention (as it usually does with my issues 😁). This might be a good thing to discuss in the meeting tomorrow.

@JacquesCarette
Copy link
Owner

As long as the external package seems 'alive', I am fine with using more external resources. And it does seem like a better solution as I completely agree that this is a display-only issue and that internally there should be no casting.

@samm82
Copy link
Collaborator Author

samm82 commented May 3, 2019

This is more of a design question - how should we decide the precision of an uncertainty (or any value, potentially)? Should it be passed in explicitly? Should we define an Uncertainty type that takes a Maybe Double (uncertainty) and Maybe Integer (precision)?

@samm82
Copy link
Collaborator Author

samm82 commented May 3, 2019

I've created a new issue for precision (#1258) so I'm closing this issue.

@samm82 samm82 closed this as completed May 3, 2019
@smiths
Copy link
Collaborator

smiths commented May 3, 2019

Yes, good idea to create a new issue.

JacquesCarette pushed a commit that referenced this issue May 10, 2019
* Updated stable use 10% instead of 10.0% for default uncertainty in GlassBR

* Display 10.0% uncertainty as 10%; as per #868

* Improved uncertainty precision implementation

* Added Uncertainty.hs

* Fixed issues with Uncertainty.hs

* Updated Language.Drasil README

* Removed extra constructor for Uncertainty

* Implemented Uncertainty throughout drasil-lang

* Added Uncertainty to Drasil.hs

* Added HasUncertainty class

* Added constraints in SSD - TODO: Add precision throughout examples

* Added default uncertainty and implemented uncertainty throughout examples

* Suppressed default-typing warning

* Linted GamePhysics UncertQs

* Fixed implementation of HasUncertainty with accessor functions

* Started moving Uncertainty to Core and fixing accessors

* Finished fixing lensing for Uncertainty

* Linted lenses
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 a pull request may close this issue.

3 participants