Added CustomSourceTime

[caseyflex]

Features:

• Validation heuristic for sufficiently fast sampling to avoid interpolation artifacts

[tylerflex]

added a few notes for discussion

[momchil-flex]

I think Tyler's question about using interp instead of sel makes sense, and also validating under-sampling.

[tylerflex]

looks good to me, let's wait to merge until @momchil-flex has a final look in conjunction with the backend tests, thanks @caseyflex !

[caseyflex]

looks good to me, let's wait to merge until @momchil-flex has a final look in conjunction with the backend tests, thanks @caseyflex !

I changed a minor thing -- allowing a dt of tolerance in covering the Simulation.tmesh, and just using the end values if needed in this case.

[tylerflex]

Just occurred to me, eventually, it would probably be useful to provide a class method to generate this source time from data in the frequency domain as well.

[tylerflex]

what is the intuition behind this choice? so the user can essentially start the time coordinates at dt and end them at run_time - dt and it should pass?

[caseyflex]

Yeah. I just found that without any tolerance on the ends, I would get errors because of floating point issues, like being just under the actual range. A dt on either side shouldn’t hurt.

…

On Thu, Jul 27, 2023 at 7:02 PM Tyler Hughes ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tidy3d/components/simulation.py <#994 (comment)>: > @@ -908,6 +913,34 @@ def _validate_tfsf_nonuniform_grid(self) -> None: f"axis, '{'xyz'[source.injection_axis]}'." ) + def _validate_customsourcetime(self) -> None: + """Make sure custom source time is not undersampled. + Also, make sure that all simulation.tmesh values are covered.""" + for source in self.sources: + if isinstance(source.source_time, CustomSourceTime): + dataset = source.source_time.source_time_dataset + if dataset is None: + continue + times = dataset.values.coords["t"].values + if ( + min(times) >= self.tmesh[0] + CUSTOMSOURCETIME_TOL * self.dt what is the intuition behind this choice? so the user can essentially start the time coordinates at dt and end them at run_time - dt and it should pass? — Reply to this email directly, view it on GitHub <#994 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A3KLECMLGNSG4QWZLDMF6LTXSKUNBANCNFSM6AAAAAA2GBD6QA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[caseyflex]

Could make it fp_eps I suppose, instead of dt On Thu, Jul 27, 2023 at 7:04 PM Charles Wojcik ***@***.***> wrote:

…

Yeah. I just found that without any tolerance on the ends, I would get errors because of floating point issues, like being just under the actual range. A dt on either side shouldn’t hurt. On Thu, Jul 27, 2023 at 7:02 PM Tyler Hughes ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In tidy3d/components/simulation.py > <#994 (comment)>: > > > @@ -908,6 +913,34 @@ def _validate_tfsf_nonuniform_grid(self) -> None: > f"axis, '{'xyz'[source.injection_axis]}'." > ) > > + def _validate_customsourcetime(self) -> None: > + """Make sure custom source time is not undersampled. > + Also, make sure that all simulation.tmesh values are covered.""" > + for source in self.sources: > + if isinstance(source.source_time, CustomSourceTime): > + dataset = source.source_time.source_time_dataset > + if dataset is None: > + continue > + times = dataset.values.coords["t"].values > + if ( > + min(times) >= self.tmesh[0] + CUSTOMSOURCETIME_TOL * self.dt > > what is the intuition behind this choice? so the user can essentially > start the time coordinates at dt and end them at run_time - dt and it > should pass? > > — > Reply to this email directly, view it on GitHub > <#994 (review)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/A3KLECMLGNSG4QWZLDMF6LTXSKUNBANCNFSM6AAAAAA2GBD6QA> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

[caseyflex]

But the tmesh already runs to run_time + dt, and it’s most natural to have the source go only to run_time On Thu, Jul 27, 2023 at 7:05 PM Charles Wojcik ***@***.***> wrote:

…

Could make it fp_eps I suppose, instead of dt On Thu, Jul 27, 2023 at 7:04 PM Charles Wojcik ***@***.***> wrote: > Yeah. I just found that without any tolerance on the ends, I would get > errors because of floating point issues, like being just under the actual > range. A dt on either side shouldn’t hurt. > > On Thu, Jul 27, 2023 at 7:02 PM Tyler Hughes ***@***.***> > wrote: > >> ***@***.**** commented on this pull request. >> ------------------------------ >> >> In tidy3d/components/simulation.py >> <#994 (comment)>: >> >> > @@ -908,6 +913,34 @@ def _validate_tfsf_nonuniform_grid(self) -> None: >> f"axis, '{'xyz'[source.injection_axis]}'." >> ) >> >> + def _validate_customsourcetime(self) -> None: >> + """Make sure custom source time is not undersampled. >> + Also, make sure that all simulation.tmesh values are covered.""" >> + for source in self.sources: >> + if isinstance(source.source_time, CustomSourceTime): >> + dataset = source.source_time.source_time_dataset >> + if dataset is None: >> + continue >> + times = dataset.values.coords["t"].values >> + if ( >> + min(times) >= self.tmesh[0] + CUSTOMSOURCETIME_TOL * self.dt >> >> what is the intuition behind this choice? so the user can essentially >> start the time coordinates at dt and end them at run_time - dt and it >> should pass? >> >> — >> Reply to this email directly, view it on GitHub >> <#994 (review)>, >> or unsubscribe >> <https://github.com/notifications/unsubscribe-auth/A3KLECMLGNSG4QWZLDMF6LTXSKUNBANCNFSM6AAAAAA2GBD6QA> >> . >> You are receiving this because you were mentioned.Message ID: >> ***@***.***> >> >

[caseyflex]

Could also have no tolerance at 0, and a dt of tolerance at run_time. On Thu, Jul 27, 2023 at 7:06 PM Charles Wojcik ***@***.***> wrote:

…

But the tmesh already runs to run_time + dt, and it’s most natural to have the source go only to run_time On Thu, Jul 27, 2023 at 7:05 PM Charles Wojcik ***@***.***> wrote: > Could make it fp_eps I suppose, instead of dt > > On Thu, Jul 27, 2023 at 7:04 PM Charles Wojcik ***@***.***> > wrote: > >> Yeah. I just found that without any tolerance on the ends, I would get >> errors because of floating point issues, like being just under the actual >> range. A dt on either side shouldn’t hurt. >> >> On Thu, Jul 27, 2023 at 7:02 PM Tyler Hughes ***@***.***> >> wrote: >> >>> ***@***.**** commented on this pull request. >>> ------------------------------ >>> >>> In tidy3d/components/simulation.py >>> <#994 (comment)> >>> : >>> >>> > @@ -908,6 +913,34 @@ def _validate_tfsf_nonuniform_grid(self) -> None: >>> f"axis, '{'xyz'[source.injection_axis]}'." >>> ) >>> >>> + def _validate_customsourcetime(self) -> None: >>> + """Make sure custom source time is not undersampled. >>> + Also, make sure that all simulation.tmesh values are covered.""" >>> + for source in self.sources: >>> + if isinstance(source.source_time, CustomSourceTime): >>> + dataset = source.source_time.source_time_dataset >>> + if dataset is None: >>> + continue >>> + times = dataset.values.coords["t"].values >>> + if ( >>> + min(times) >= self.tmesh[0] + CUSTOMSOURCETIME_TOL * self.dt >>> >>> what is the intuition behind this choice? so the user can essentially >>> start the time coordinates at dt and end them at run_time - dt and it >>> should pass? >>> >>> — >>> Reply to this email directly, view it on GitHub >>> <#994 (review)>, >>> or unsubscribe >>> <https://github.com/notifications/unsubscribe-auth/A3KLECMLGNSG4QWZLDMF6LTXSKUNBANCNFSM6AAAAAA2GBD6QA> >>> . >>> You are receiving this because you were mentioned.Message ID: >>> ***@***.***> >>> >>

[tylerflex]

A few things:

• The point of this validator is to ensure that the user-supplied data is enough to cover the tmesh right? so this change seems to reduce the size of the range from (tmesh[0], tmesh[-1]) to (tmesh[0] + 1.1 dt, tmesh[-1] - 1.1dt). Basically with this change, it seems a could not supply data for t=0 and a nan would get created on our side right? That seems problematic.
• the tmesh is np.arange(0.0, self.run_time + dt, dt), np.arange doesnt include the final value so the final value is actually self.run_time, just FYI.

So the problem you're having is that if a user supplies run_time=x to the Simulation and then makes a t coordinate grid for the custom time source dataset using np.linspace(0, run_time, N), the validator will sometimes complain due to numerical precision? If so, maybe one option is to relax the validator for the [0] and [-1] values by -fp_eps and +fp_eps (opposite direction as current?) and then make sure to also apply this on the solver side?

[caseyflex]

A few things:

• The point of this validator is to ensure that the user-supplied data is enough to cover the tmesh right? so this change seems to reduce the size of the range from (tmesh[0], tmesh[-1]) to (tmesh[0] + 1.1 dt, tmesh[-1] - 1.1dt). Basically with this change, it seems a could not supply data for t=0 and a nan would get created on our side right? That seems problematic.
• the tmesh is np.arange(0.0, self.run_time + dt, dt), np.arange doesnt include the final value so the final value is actually self.run_time, just FYI.

So the problem you're having is that if a user supplies `run_time=x` to the Simulation and then makes a `t` coordinate grid for the custom time source dataset using `np.linspace(0, run_time, N)`, the validator will sometimes complain due to numerical precision? If so, maybe one option is to relax the validator for the [0] and [-1] values by -fp_eps and +fp_eps (opposite direction as current?) and then make sure to also apply this on the solver side?

I changed source.py to also account for these edge cases, and produce the end values instead of nan. I also added a test for this. I think the signs on the tolerance are correct.

Can you look at the most recent version? I think 0 tolerance at the start time and dt tolerance at the end time is reasonable, since we warn for sampling below dt anyway, it is fair to extrapolate one dt past the stop time. Although fp_eps would also be fine at the stop time.

[tylerflex]

Alright momchil pointed me to the backend code and the handling for out of bounds and that looks fine. I thought for a second that we just evaluated the amp function server side and injected whatever came out of that. I think from my perspective it looks good to go.