Resolve "Clock noise is not included in `tcb_timer_deviations`"

Closes #87 (closed)

• Fix the TCB timer deviations (using local timer deviations to include clock noise)
• Refactor TCB synchronization noise to its own function in noise submodule

changed milestone to %v1.1

added bug label

added 2 commits

• b29f8d4d - Fix a bug with TCB timer deviations
• 79e2064b - Define TCB synchronization noise

Compare with previous version

marked this merge request as ready

changed the description

requested review from @Nik

Hi @Nik.

A bug we seem to have overlooked earlier. Could you take a look?

Hi @j2b.bayle

I think the timer offsets were included in the tcb timer deviations and also in the local timer deviations.

Regarding your changes: as far as I understand: in the method cumulative_trapezoid the parameter initial unfortunately does not refer to an integration constant, which would be logical, but to a value inserted at position 0. So I guess we cannot set initial=self.clock_offsets[sc] and should add it outside the function as before.

But the tcb_timer_deviations indeed have to be changed, because the stochastic clock noise is not included. I will try to do that tomorrow within issue #68 (which is on my list far too long, I am sorry for the delay)

Oh sorry, that's what was missing.

I had noticed a bug last week and committed a few changes -- but misremembered them.

Let me fix the issue title and the content of the MR.

changed title from Resolve "Timer offsets are not included in tcb_timer_deviations" to Resolve "Clock noise is not included in tcb_timer_deviations"

changed the description

added 2 commits

• d8a0fc71 - Add clock jitter noise to TCB timer deviations
• c16165a2 - Refactor TCB sync noise into its own function

Compare with previous version

Alright, this should work. Can you check?

added 2 commits

• 52aa9b27 - Add clock jitter noise to TCB timer deviations
• b0f36070 - Refactor TCB sync noise into its own function

Compare with previous version

There is one problem left: tcb_sync_noises is sampled according the telemetry_times but local_timer_deviations according to the physics sampling rate.

I think we need to write something like self.local_timer_deviations[sc][::int(self.physics_upsampling * self.telemetry_downsampling)]

And also the tps_proper_time_deviations probably have to be resampled

You're right. I've also noticed that problem (which pre-existed this MR) for very long simulations, when we simulate more than one telemetry period. Maybe that can be addressed in a separate MR?

If that's okay with you, maybe you can create an issue about this? Or we include it in #68?

That is okay for me:) Would you prefer a new issue or inclusion in #68?

approved this merge request

Let’s do a new issue that we can quickly merge. It’s more of a bug fix.