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

Feature: Add LapseRate extensions #324

Merged
merged 1 commit into from
Nov 26, 2017
Merged

Feature: Add LapseRate extensions #324

merged 1 commit into from
Nov 26, 2017

Conversation

jnyrup
Copy link
Contributor

@jnyrup jnyrup commented Nov 20, 2017

This PR extends LapseRate with four operator overloads:

  • TemperatureDelta / LapseRate -> Length
  • Temperature / LapseRate -> Length
  • Length * LapseRate -> TemperatureDelta
  • LapseRate * Length -> TemperatureDelta

@0xferit 0xferit self-requested a review November 21, 2017 09:25
Copy link
Contributor

@0xferit 0xferit left a comment

Choose a reason for hiding this comment

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

Welcome and thanks for the contribution. I couldn't find any errors but let's wait for @angularsen for merging.

@jnyrup
Copy link
Contributor Author

jnyrup commented Nov 21, 2017

What are your gut feelings about the Temperature / LapseRate -> Length?
Will it cause more harm than good based on previous confusion related to the difference between Temperature and TemperatureDelta.

And similar, how about conversions to LapseRate?

I think TemperatureDelta / Length -> LapseRate makes perfect sense, while Temperature / Length -> LapseRate might too ambiguous?

For consistency I could imagine we wanted to either only use TemperatureDelta or use Temperature in both conversions from and to LapseRate.

Any preferences on whether TemperatureDelta / Length -> LapseRate is an extension to TemperatureDelta or Length?

@angularsen
Copy link
Owner

If I understand correctly, I think Temperature cannot be used for the same reason you can't add two temperatures, you can only add a delta (range on scale) to a temperature (point on scale) or add two deltas to get a new larger delta.

LapseRate [▲°C/m] describes the gradient for how much the temperature changes (delta) over a distance, so the proposed equation becomes:

TemperatureDelta [▲°C] / LapseRate [▲°C/m] = 
TemperatureDelta [▲°C] / (TemperatureDelta [▲°C] / Length [m]) = 
Length [m]

In hindsight, LapseRate abbreviation should probably be changed to reflect this? It is currently missing the delta symbol. But not sure if this goes against the convention in the domain.

Any preferences on whether TemperatureDelta / Length -> LapseRate is an extension to TemperatureDelta or Length?

I would say TemperatureDelta, because it is the leading quantity.

return Length.FromKilometers(left.KelvinsDelta / right.DegreesCelciusPerKilometer);
}

public static Length operator /(Temperature left, LapseRate right)
Copy link
Owner

Choose a reason for hiding this comment

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

I vote to drop this.

Copy link
Owner

Choose a reason for hiding this comment

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

If this is removed, I think this PR is good to go.

@jnyrup
Copy link
Contributor Author

jnyrup commented Nov 24, 2017

Thanks for the review.
I've updated the PR with two changes:

  • removed the Temperate / LapseRate -> Length
  • added TemperatureDelta / Length -> LapseRate

Regarding the abbreviation:

  • Currently it is °C/m for LapseRateUnit.DegreeCelsiusPerKilometer, which is wrong due to a missing a k.
  • ▲°C/km would be fine with me.

@0xferit
Copy link
Contributor

0xferit commented Nov 24, 2017

@jnyrup About missing k, yeah I did implement that and when I changed base unit forgot to change the abbreviation also, my bad.

And yeah, we should change the abbreviation to ▲°C/km

@jnyrup
Copy link
Contributor Author

jnyrup commented Nov 24, 2017

And related, why do we use ▲ instead of ∆?

@0xferit
Copy link
Contributor

0xferit commented Nov 24, 2017

I'm not aware of a specific reason for using ▲. I used ▲ because @angularsen used it in his comment above. Let's wait for his answer.

@angularsen
Copy link
Owner

No preference for me other than it already being used for TemperatureDelta, I suppose ∆ is more correct.

@0xferit
Copy link
Contributor

0xferit commented Nov 24, 2017

Hmm, then I'm opening an issue for replacing every occurrence of ▲ with ∆ in the library, for the sake of consistency.

I also see some minor differences in code-style in various places (talking in general, not about this p.r.). Perhaps we can set a standard on this and write it to the wiki. Standards make contributing less confusing and reviewing less difficult, I believe.

@angularsen
Copy link
Owner

That would be good! And maybe adding it as a contribution guideline so it pops up whenever someone creates a new issue / PR.

@0xferit 0xferit mentioned this pull request Nov 24, 2017
@jnyrup
Copy link
Contributor Author

jnyrup commented Nov 25, 2017

I've updated the PR so the unit of lapse rate is now ▲°C/km.

@angularsen
Copy link
Owner

@jnyrup Shouldn't we use the new delta symbol? Per #328 .

@jnyrup
Copy link
Contributor Author

jnyrup commented Nov 25, 2017

I thought that was the job of #327?

@angularsen
Copy link
Owner

angularsen commented Nov 25, 2017

Sure, but if you are already modifying the abbreviation here, why not use the correct symbol to begin with?
If you change the symbol, this PR is ready to merge.

@jnyrup
Copy link
Contributor Author

jnyrup commented Nov 25, 2017

The PR has been updated with the correct delta symbol.
Thanks for the fast reviews!

@angularsen angularsen merged commit b1e372d into angularsen:master Nov 26, 2017
@jnyrup jnyrup deleted the feature/LapseRate-extensions branch November 26, 2017 12:12
@angularsen
Copy link
Owner

Thanks! Nuget 3.81 is out.

@dglozano dglozano mentioned this pull request Nov 16, 2021
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.

3 participants