Added units for Astronomy. #680
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
angularsen
requested changes
Jul 18, 2019
angularsen
reviewed
Jul 18, 2019
|
<!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
code
{mso-style-priority:99;
font-family:"Courier New";}
pre
{mso-style-priority:99;
mso-style-link:"Pr\00E9format\00E9 HTML Car";
margin:0cm;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";}
span.PrformatHTMLCar
{mso-style-name:"Pr\00E9format\00E9 HTML Car";
mso-style-priority:99;
mso-style-link:"Pr\00E9format\00E9 HTML";
font-family:"Courier New";}
.MsoChpDefault
{mso-style-type:export-only;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 90.0pt 72.0pt 90.0pt;}
div.WordSection1
{page:WordSection1;}
-->I don’t have it defined for Length for AU, and that’s where it crashes with inches. Provenance : Courrier pour Windows 10 De : Andreas Gullberg LarsenEnvoyé le :17 juillet 2019 21:38À : angularsen/UnitsNetCc : Étienne Fortin; AuthorObjet :Re: [angularsen/UnitsNet] Added units for Astronomy. (#680) @angularsen commented on this pull request.In Common/UnitDefinitions/Power.json:> @@ -94,6 +94,19 @@ "Abbreviations": [ "Btu/hr" ] } ]+ },+ {+ "SingularName": "SolarLuminosity",+ "PluralName": "SolarLuminosities",+ "BaseUnit": "Watt",I think this is the culprit of your unit test errors. Units should not have "BaseUnit" defined, only quantities, so you can remove this.—You are receiving this because you authored the thread.Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Info.GetUnitInfosFor() returns an enumration. In it I get Meters (for Length) and all of my new defined units. So I guess I should remove BaseUnits as well? Not only BaseUnit? |
|
<!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
code
{mso-style-priority:99;
font-family:"Courier New";}
pre
{mso-style-priority:99;
mso-style-link:"Pr\00E9format\00E9 HTML Car";
margin:0cm;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";}
span.PrformatHTMLCar
{mso-style-name:"Pr\00E9format\00E9 HTML Car";
mso-style-priority:99;
mso-style-link:"Pr\00E9format\00E9 HTML";
font-family:"Courier New";}
.MsoChpDefault
{mso-style-type:export-only;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 90.0pt 72.0pt 90.0pt;}
div.WordSection1
{page:WordSection1;}
-->Ok now it works. You were right. BaseUnits need not be defined if I’m inheriting from the base unit defined for the quantity. Provenance : Courrier pour Windows 10 De : [email protected]é le :17 juillet 2019 21:39À : angularsen/UnitsNet; angularsen/UnitsNetCc : AuthorObjet :RE: Re: [angularsen/UnitsNet] Added units for Astronomy. (#680) I don’t have it defined for Length for AU, and that’s where it crashes with inches. Provenance : Courrier pour Windows 10 De : Andreas Gullberg LarsenEnvoyé le :17 juillet 2019 21:38À : angularsen/UnitsNetCc : Étienne Fortin; AuthorObjet :Re: [angularsen/UnitsNet] Added units for Astronomy. (#680) @angularsen commented on this pull request.In Common/UnitDefinitions/Power.json:> @@ -94,6 +94,19 @@ "Abbreviations": [ "Btu/hr" ] } ]+ },+ {+ "SingularName": "SolarLuminosity",+ "PluralName": "SolarLuminosities",+ "BaseUnit": "Watt",I think this is the culprit of your unit test errors. Units should not have "BaseUnit" defined, only quantities, so you can remove this.—You are receiving this because you authored the thread.Reply to this email directly, view it on GitHub, or mute the thread.
|
|
<!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
code
{mso-style-priority:99;
font-family:"Courier New";}
pre
{mso-style-priority:99;
mso-style-link:"Pr\00E9format\00E9 HTML Car";
margin:0cm;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";}
span.PrformatHTMLCar
{mso-style-name:"Pr\00E9format\00E9 HTML Car";
mso-style-priority:99;
mso-style-link:"Pr\00E9format\00E9 HTML";
font-family:"Courier New";}
.MsoChpDefault
{mso-style-type:export-only;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 90.0pt 72.0pt 90.0pt;}
div.WordSection1
{page:WordSection1;}
-->Yeah I’m still debating with myself. Problem is in Astronomy Luminosity is defined in Watts only, Nothing else. So I have a hard time converting it to Lumens or Candela. Provenance : Courrier pour Windows 10 De : Andreas Gullberg LarsenEnvoyé le :17 juillet 2019 21:54À : angularsen/UnitsNetCc : Étienne Fortin; AuthorObjet :Re: [angularsen/UnitsNet] Added units for Astronomy. (#680) @angularsen commented on this pull request.In Common/UnitDefinitions/Power.json:> @@ -94,6 +94,19 @@ "Abbreviations": [ "Btu/hr" ] } ]+ },+ {+ "SingularName": "SolarLuminosity",https://en.wikipedia.org/wiki/LuminosityI guess there is some overlap in units (Watt, joules per second), but it does seem like Luminosity is considered a separate type of quantity since they define it as electromagnetic power. What do you think?—You are receiving this because you authored the thread.Reply to this email directly, view it on GitHub, or mute the thread.
|
|
It's very late over here and I have to go now, will try to come back to this tomorrow. |
|
<!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
.MsoChpDefault
{mso-style-type:export-only;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 90.0pt 72.0pt 90.0pt;}
div.WordSection1
{page:WordSection1;}
-->No problem. Talk to you tomorrow. Provenance : Courrier pour Windows 10 De : Andreas Gullberg LarsenEnvoyé le :17 juillet 2019 21:55À : angularsen/UnitsNetCc : Étienne Fortin; AuthorObjet :Re: [angularsen/UnitsNet] Added units for Astronomy. (#680) It's very late over here and I have to go now, will try to come back to this tomorrow.—You are receiving this because you authored the thread.Reply to this email directly, view it on GitHub, or mute the thread.
|
|
<!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
code
{mso-style-priority:99;
font-family:"Courier New";}
pre
{mso-style-priority:99;
mso-style-link:"Pr\00E9format\00E9 HTML Car";
margin:0cm;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";}
span.PrformatHTMLCar
{mso-style-name:"Pr\00E9format\00E9 HTML Car";
mso-style-priority:99;
mso-style-link:"Pr\00E9format\00E9 HTML";
font-family:"Courier New";}
.MsoChpDefault
{mso-style-type:export-only;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 90.0pt 72.0pt 90.0pt;}
div.WordSection1
{page:WordSection1;}
-->But yes, having an electromagnetic power quantity would make sense. Provenance : Courrier pour Windows 10 De : [email protected]é le :17 juillet 2019 21:55À : angularsen/UnitsNet; angularsen/UnitsNetCc : AuthorObjet :RE: Re: [angularsen/UnitsNet] Added units for Astronomy. (#680) Yeah I’m still debating with myself. Problem is in Astronomy Luminosity is defined in Watts only, Nothing else. So I have a hard time converting it to Lumens or Candela. Provenance : Courrier pour Windows 10 De : Andreas Gullberg LarsenEnvoyé le :17 juillet 2019 21:54À : angularsen/UnitsNetCc : Étienne Fortin; AuthorObjet :Re: [angularsen/UnitsNet] Added units for Astronomy. (#680) @angularsen commented on this pull request.In Common/UnitDefinitions/Power.json:> @@ -94,6 +94,19 @@ "Abbreviations": [ "Btu/hr" ] } ]+ },+ {+ "SingularName": "SolarLuminosity",https://en.wikipedia.org/wiki/LuminosityI guess there is some overlap in units (Watt, joules per second), but it does seem like Luminosity is considered a separate type of quantity since they define it as electromagnetic power. What do you think?—You are receiving this because you authored the thread.Reply to this email directly, view it on GitHub, or mute the thread.
|
angularsen
requested changes
Jul 19, 2019
|
Regarding Luminosity, the units in Astronomy is really Watts. Can I have two quantities that resolve to the same Watts Unit? How would that work when we do calculation between units? Will the framework understands they resolve both to Watts? Or will two different Watts exist? |
|
Watts can exist in both Power and Luminosity, no problem in itself. Parse() and ToString() will work for both. We can add helper methods and properties to convert between Power and Luminosity such as Luminosity.ToPower(). This way luminosity specific units are in Luminosity. However, I am no domain expert and this comes down to what is the most intuitive to those (you) who will be using it. |
|
I will do a new Quantity then. Makes more sense and from what you say will work. |
|
If you like you can put the inverse values in a comment next to it.
|
|
REgarding adding new quantities, I'm stuck at the Test class. generate-code do not create the test class. And doc talks about a batch file "GenerateUnits.bat" that I can't find anywhere. |
|
Forget it, it generates the code. I had a typo somewhere so there was a filename mismatch. |
DO NOT CURRENTLY COMPILE!
|
Please look at how I've added the Luminosity Quantity. It doesn't compile. There are multiple methods with the same name generated. And my SolarLuminosity unit doesn't appears in code. |
angularsen
reviewed
Jul 19, 2019
It seems you have duplicate JSON files:
|
Merge from origin
All tests passes.
|
I see there is still an error in my conversion constants for Solar mass. I maybe didn't execute generate-code after the last change. Errors are in generated code. |
angularsen
requested changes
Jul 20, 2019
Ah, yes, it can be a good idea to run |
|
Let me know when you are done making changes and I can review again. |
|
We should be good with my latest commit. |
angularsen
approved these changes
Jul 20, 2019
|
Looks good to me 🎉 Nuget on the way out. |
ebfortin
added a commit
to ebfortin/UnitsNet
that referenced
this pull request
Jul 20, 2019
Added units for Astronomy. (angularsen#680)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request for checking problem with inches unit tests being "polluted" by new astronomy units added.