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

Time brightness #580

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Time brightness #580

wants to merge 6 commits into from

Conversation

cquiroz
Copy link
Collaborator

@cquiroz cquiroz commented Sep 11, 2022

This is a proposal for adding a time dimension to Magnitudes
Basically it replaces measurement values with NonEmptyMap[Timestamp, BrightnessMeasure[T]]

NonEmptyMap is backed by a SortedMap thus we'd keep the entries in order and we need at least one, signalling the common case where the value is constant

There are a few things missing, like interpolation of values and lenses but wanted to request a first impression on the impact of this change.

@tpolecat
Copy link
Member

Seems reasonable to me. Due to the similarity with time-varying coordinates it will be tempting to abstract out TImeVarying[A], but I think we should wait on doing so until we're happy with the way everything is working (by that point it may not make sense to generalize).

Copy link
Contributor

@swalker2m swalker2m left a comment

Choose a reason for hiding this comment

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

Source profile is already so sophisticated that it pains me to add more but we do need to match the reality of the situation. We'll need to use the time dimension for AGS magnitude look ups and, presumably, ITC calculations so this will ripple out a bit.

@@ -19,6 +22,7 @@ object BrightnessUnits {
trait FluxDensityContinuum[+T]

type BrightnessMeasure[T] = Measure[BigDecimal] Of Brightness[T]
type BrightnessMeasureOverTime[T] = NonEmptyMap[Instant, Measure[BigDecimal] Of Brightness[T]]
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we could use the BrightnessMeasure alias here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds like a good idea, to reduce confusion a bit

final case class EmissionLine[T](
lineWidth: Quantity[PosBigDecimal, KilometersPerSecond],
lineFlux: Measure[PosBigDecimal] Of LineFlux[T]
lineFlux: EmissionLine.LineFluxOverTime[T]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know anything about this unfortunately. It makes sense that the "line flux" would vary but are we sure that it does?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll verify, my input from science is that emission lines can also vary over time

}

object EmissionLine {
implicit def eqEmissionLine[T]: Eq[EmissionLine[T]] =
type LineFluxOverTime[T] = NonEmptyMap[Instant, Measure[PosBigDecimal] Of LineFlux[T]]
Copy link
Contributor

Choose a reason for hiding this comment

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

I wondered why we don't add a LineFluxMeasure to parallel the BrightnessMeasure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like I just missed it when implementing emission lines. Let's add it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants