feat: add validation for units

[rahul-flex]

Added setup errors for grid spacing dl and frequency (attached).

[notion-workspace]

Sanity check for units in simulation setup

[yaugenst-flex]

We encountered users specifying the wrong units in their simulation setup, so the motivation here is to introduce range-based validators and link to the relevant docs page to make sure users can quickly identify and correct unit issues. While the validators impose strict bounds, the idea is not to necessarily allow those values but simply catch setups that are clearly off by orders of magnitude. We had a stab at selecting bounds that would cover that but if you could double check @momchil-flex and @tomflexcompute?

[momchil-flex]

I think it makes sense, the main concern is RF freqs, it seems like the warning will currently be raised for a wavelength larger than 1 meter, which is probably fine for all applications we're currently targeting? But may be close. But probably for a center frequency it's fine.

I remember we added a similar validator at some point, maybe only for the mode solver... But I can't find it now. @weiliangjin2021 do you remember?

[yaugenst-flex]

Thanks @rahul-flex this looks good! Left some minor comments. Make sure to also add tests for these validators and a changelog entry (under the "Changed" section probably).

[yaugenst-flex]

it seems like the warning will currently be raised for a wavelength larger than 1 meter, which is probably fine for all applications we're currently targeting? But may be close.

Hmm I guess that also means we might want dl > 1cm? @momchil-flex

[tomflexcompute]

This is nice but I think 1m wavelength is actually a bit too restrictive for some use cases like radar and long range communication, even though these use cases are not common within our current user base.

[yaugenst-flex]

@tomflexcompute I guess that would put us in the ~100m wavelength range?

[tomflexcompute]

@tomflexcompute I guess that would put us in the ~100m wavelength range?

yeah potentially. It's probably fine for now but in the future if we have more users from RF and microwave it might be a problem.

[weiliangjin2021]

We have a validator here checking the minimal frequency. The lowest frequency is 1e5 Hz (here).

It's applied to FreqMonitor here, and source here.

[rahul-flex]

Thanks for the comments @yaugenst-flex @momchil-flex @tomflexcompute @weiliangjin2021. Should we make upper bound of dl > 1cm (say 10 cm?) and lower bound of frequency: 1e5 Hz?

[weiliangjin2021]

Should we make upper bound of dl > 1cm (say 10 cm?) and lower bound of frequency: 1e5 Hz?

I wonder if we really need a validator for grid size. If the grid size is too large, you'll get just one grid in that dimension, and it should be easy to spot the issue? And if the grid size is too small, I think there is already a validator that checks the total number of grid points below a threshold value.

Now if we go for a validator for grid size, with lower bound frequency 1e5 Hz, corresponding to wavelength 300m, the upper bound of grid size will be 30 m.

[rahul-flex]

And if the grid size is too small, I think there is already a validator that checks the total number of grid points below a threshold value.

@weiliangjin2021 Thanks for the bound values.
I’m not aware of an existing validator for grid size. Currently, the error for fine grid
seems to be due to high memory allocation, as reported by a user ( screenshot attached).

[weiliangjin2021]

And if the grid size is too small, I think there is already a validator that checks the total number of grid points below a threshold value.

@weiliangjin2021 Thanks for the bound values. I’m not aware of an existing validator for grid size. Currently, the error for fine grid seems to be due to high memory allocation, as reported by a user ( screenshot attached).

See

tidy3d/tidy3d/components/simulation.py

Lines 3701 to 3709 in 14f326d

	def _validate_size(self) -> None:
	"""Ensures the simulation is within size limits before simulation is uploaded."""
	num_comp_cells = self.num_cells / 2 ** (np.sum(np.abs(self.symmetry)))
	if num_comp_cells > MAX_GRID_CELLS:
	raise SetupError(
	f"Simulation has {num_comp_cells:.2e} computational cells, "
	f"a maximum of {MAX_GRID_CELLS:.2e} are allowed."
	)

. But it seems it errorred already before reaching here.

[rahul-flex]

. But it seems it errorred already before reaching here.

Ohh, I see! Thanks!

[yaugenst-flex]

And if the grid size is too small, I think there is already a validator that checks the total number of grid points below a threshold value.

@weiliangjin2021 Thanks for the bound values. I’m not aware of an existing validator for grid size. Currently, the error for fine grid seems to be due to high memory allocation, as reported by a user ( screenshot attached).

See

tidy3d/tidy3d/components/simulation.py

Lines 3701 to 3709 in 14f326d

	def _validate_size(self) -> None:
	"""Ensures the simulation is within size limits before simulation is uploaded."""
	num_comp_cells = self.num_cells / 2 ** (np.sum(np.abs(self.symmetry)))
	if num_comp_cells > MAX_GRID_CELLS:
	raise SetupError(
	f"Simulation has {num_comp_cells:.2e} computational cells, "
	f"a maximum of {MAX_GRID_CELLS:.2e} are allowed."
	)

. But it seems it errorred already before reaching here.

I think the size validator is not quite the same though, or at least if there is an opportunity to catch a bad dl earlier that'd still be beneficial. Actually we could also just change the dl validator have no upper but only a lower bound.

[yaugenst-flex]

Thanks @rahul-flex

@weiliangjin2021 fyi we ended up going conservative and only check for tiny grid spacing now. This check would've already caught the error the user that prompted this PR was seeing.

[weiliangjin2021]

@weiliangjin2021 fyi we ended up going conservative and only check for tiny grid spacing now. This check would've already caught the error the user that prompted this PR was seeing.

Looks nice! A minor: "if not (1e-7 <= val)" --> "if val <1e-7" would be easier to read

[rahul-flex]

Thanks @weiliangjin2021 and @yaugenst-flex for the comments. Simplified the condition.

[yaugenst-flex]

Thanks @rahul-flex looks good to go but could you please squash this into a single commit before we merge and make sure the changelog entry is in the right place. currently it's listed as a changelog item under 2.8.2 (should go under "unreleased")

[momchil-flex]

@yaugenst-flex @rahul-flex a user just had some simulations erroring because they had obviously set units wrong, which resulted in a grid spacing on the scale of 1e-9. However, this was still an auto grid (they had also set source frequency wrong), so it was not caught by this validator. A large number of our users probably use auto grid instead of uniform grid - it would be good to have a similar validator there. However, in that case it should be a Simulation post_init validator, as the grid needs to be constructed to examine the smallest dl.