Rework make_filter interface to accept either size_px or radius and d… #1776

yaugenst-flex · 2024-06-19T15:00:05Z

…l arguments, import all user-facing functions into the plugins/autograd namespace, and add tests

tylerflex

Thanks @yaugenst . Looks good overall, a few details.
The main thing to discuss is the fact that this breaks backwards compabibility with the size -> size_px change. What are your thoughts on that? is autograd so experimental that we can do this safely?

tylerflex · 2024-06-19T15:04:39Z

tidy3d/plugins/autograd/invdes/filters.py

+        kernel_size = get_kernel_size_px(radius=radius, dl=dl)
+        return (kernel_size,) if np.isscalar(kernel_size) else tuple(kernel_size)
+    else:
+        raise ValueError("Either 'size_px' or both 'radius' and 'dl' must be provided.")


to remove any ambiguity, it might be best to just only handle specific cases?

if size_px is not None and (radius is None and dl is None): return (size_px,) if np.isscalar(size_px) else tuple(size_px) elif radius is not None and dl is not None and size_px is None: kernel_size = get_kernel_size_px(radius=radius, dl=dl) return (kernel_size,) if np.isscalar(kernel_size) else tuple(kernel_size) else: raise ValueError("Either 'size_px' or both 'radius' and 'dl' must be provided.")

Ah, my thinking was that if a kernel size in pixels is given, then that would take precedence over radius and dl. Maybe it makes sense to do this and warn the user? Or just error?

up to you. but just need to clearly define what happens if all 3 are provided perhaps. I might suggest error or at least warn? seems like untented use.

It is clearly defined, no? If size_px is given then that takes precedence over everything else. The reason being that if you supply an explicit size in pixels for the filter, then you probably do not want to calculate a different size from other arguments? Maybe it's enough to add this to the docstring?
I thought this was a relatively common pattern, see for example the period argument in np.interp.

ok that's fine with me. I tend to be a bit conservative about things like this (tend towards erroring), if only to make things simpler, but if it's ok for numpy then I'm fine with it.

tidy3d/plugins/autograd/invdes/filters.py

tidy3d/plugins/autograd/utilities.py

tidy3d/plugins/autograd/__init__.py

CHANGELOG.md

yaugenst-flex · 2024-06-19T15:21:14Z

The main thing to discuss is the fact that this breaks backwards compabibility with the size -> size_px change. What are your thoughts on that? is autograd so experimental that we can do this safely?

Yeah this is pretty unfortunate, but size is I think too ambiguous... I think (I hope) it's fine? I'm sure there will be other breaking changes too...

tylerflex · 2024-06-28T14:37:49Z

Let's change target of this to pre/2.8?

yaugenst-flex · 2024-06-28T14:39:16Z

Oh yeah was just in the process of doing that :)

…l arguments, import all user-facing functions into the plugins/autograd namespace, and add tests

tylerflex

Looks pretty good.

I'm having trouble visualizing the API / if there are any changes? especially to invdes? does it break backwards compatibility?

I also think it would make some sense to define the default parameters, especially for beta, as constants somewhere so we can tweak them more easily.

Thanks @yaugenst-flex

tylerflex · 2024-07-10T18:39:03Z

CHANGELOG.md

@@ -17,6 +17,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 ### Changed

+### Changed


duplicate changelog?

Also maybe add a line saying that all user-facing features can be imported now directly from tidy3d.plugins.autograd?

tylerflex · 2024-07-10T18:39:29Z

docs/faq

will need to update this submodule to latest state of develop

tylerflex · 2024-07-10T18:40:28Z

tidy3d/plugins/autograd/README.md

@@ -152,6 +152,8 @@ Next on our roadmap (targeting 2.8 and 2.9, summer 2024) is to support:
 - `PoleResidue` and other dispersive models.
 - custom (spatially-dependent) dispersive models, allowing topology optimization with metals.

+- `ComplexPolySlab`


remove this change? also, wondering if you ever updated this readme to reflect the field data operations (put them in the "supported" section?) if not, maybe we can do that in this PR?

idea is to have this README contain the latest state of released, in progress, and todo.

tylerflex · 2024-07-10T18:41:22Z

tidy3d/plugins/autograd/functions.py

these changes exist already in develop from the interp PR? should we merge develop into pre/2.8 maybe?

Oh yeah I guess they do. Does it maybe make sense to keep working on pre/2.8 for the features and then cherry-pick things for patches to develop? That would prevent the branches from diverging too much.

I'm not sure, guess first are we committed to merging this into 2.8 or should we just get it into develop? no real preference here.

Then, I think we generally merge develop into the pre branches before release, so it will happen eventually.

tylerflex · 2024-07-10T18:41:56Z

tidy3d/plugins/autograd/invdes/filters.py

+
+
+class AbstractFilter(Tidy3dBaseModel, abc.ABC):
+    """A filter class for creating and applying convolution filters.


maybe just An abstract class since filter is mentioned later?

tylerflex · 2024-07-10T18:53:11Z

tidy3d/plugins/autograd/invdes/parametrizations.py

+        else:
+            raise ValueError(f"Unsupported filter_type: {filter_type}")
+
+        if size_px is not None:


I thought we had something that handles this for us? yea basically maybe all of this could be make_filter(...)?

The reason for this is that I thought that we might be getting rid of the functional interface, i.e. make_filter(). If you think we should keep it then I will reuse it here.

if it doesn't re implement the same code, I'm fine keeping it.

tylerflex · 2024-07-10T18:53:58Z

tidy3d/plugins/autograd/invdes/penalties.py


-    def _dilate(array: np.ndarray, beta: float):
-        return filtproj(array, beta=beta, eta=eta_dilate)
+    filtproj: FilterAndProject


use pd.Field instead of docstrings

tylerflex · 2024-07-10T18:54:28Z

tidy3d/plugins/autograd/invdes/penalties.py

-    -------
-    Callable
-        A function that computes the erosion/dilation penalty for a given array.
+    filtproj : FilterAndProject


hm, this is an interesting approach.. but maybe we need a good default value for this based on the default penalty params?

Ah you mean not having the parametrization here be a customizable thing but built into the penalty? We can do that, wasn't sure which way is best.

yea or (similar comment above) we can make the parameters (beta, eta, etc.) be the pd.Fields here.

with good defaults

tylerflex · 2024-07-10T18:55:29Z

tidy3d/plugins/autograd/invdes/penalties.py

+        eta: float = 0.5,
+        delta_eta: float = 0.01,
+        padding: PaddingType = "reflect",
+    ) -> "ErosionDilationPenalty":


use future annotations

tylerflex · 2024-07-10T18:55:48Z

tidy3d/plugins/autograd/invdes/penalties.py

+        dl: Union[float, Tuple[float, ...]] = None,
+        *,
+        size_px: Union[int, Tuple[int, ...]] = None,
+        beta: float = 100.0,


in my recent experience, this is probably too high. I might recommend like 20.. not sure if this breaks backwards compatibility technically

It would, because that default is used in a bunch of notebooks 😀 should we change it anyways?

probably... but perhaps a goal before 2.8 should be to figure out very good defaults for these things.

yaugenst-flex · 2024-07-11T13:43:24Z

I'm having trouble visualizing the API / if there are any changes? especially to invdes? does it break backwards compatibility?

It doesn't break backwards compatibility, no, only if we decide to get rid of the functional interface altogether.

tylerflex · 2024-07-11T13:57:16Z

let's leave the functional interface but introduce these new classes and maybe change notebooks to use the classes?

yaugenst-flex requested a review from tylerflex June 19, 2024 15:00

yaugenst-flex force-pushed the yaugenst-flex/autograd-utilities branch from 41d0ba0 to 46b6f2e Compare June 19, 2024 15:06

tylerflex reviewed Jun 19, 2024

View reviewed changes

yaugenst-flex force-pushed the yaugenst-flex/autograd-utilities branch from 46b6f2e to 0923c79 Compare June 19, 2024 15:09

yaugenst-flex force-pushed the yaugenst-flex/autograd-utilities branch from c145880 to 99c0cab Compare June 28, 2024 14:33

yaugenst-flex changed the base branch from develop to pre/2.8 June 28, 2024 14:39

yaugenst-flex force-pushed the yaugenst-flex/autograd-utilities branch from 99c0cab to 744f377 Compare July 1, 2024 10:29

yaugenst-flex self-assigned this Jul 2, 2024

tylerflex and others added 4 commits July 8, 2024 16:08

autograd geometry group

dbb959d

Rework make_filter interface to accept either size_px or radius and d…

159fd85

…l arguments, import all user-facing functions into the plugins/autograd namespace, and add tests

Add get_kernel_size_px docstrings

1fb2745

Update docstrings to use val : type = default syntax and update types

0ab7d2c

yaugenst-flex force-pushed the yaugenst-flex/autograd-utilities branch from 744f377 to 0ab7d2c Compare July 8, 2024 14:21

Add class-based filters

c2352d0

yaugenst-flex requested a review from tylerflex July 9, 2024 16:51

Add class-based penalties and parametrizations

898f4c2

yaugenst-flex force-pushed the yaugenst-flex/autograd-utilities branch from ff9f499 to 898f4c2 Compare July 9, 2024 16:57

Fix tests

4b41d2a

tylerflex reviewed Jul 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework make_filter interface to accept either size_px or radius and d… #1776

Rework make_filter interface to accept either size_px or radius and d… #1776

yaugenst-flex commented Jun 19, 2024

tylerflex left a comment

tylerflex Jun 19, 2024

yaugenst-flex Jun 19, 2024

tylerflex Jul 10, 2024

yaugenst-flex Jul 11, 2024

tylerflex Jul 11, 2024

yaugenst-flex commented Jun 19, 2024

tylerflex commented Jun 28, 2024

yaugenst-flex commented Jun 28, 2024

tylerflex left a comment

tylerflex Jul 10, 2024

tylerflex Jul 10, 2024

tylerflex Jul 10, 2024

tylerflex Jul 10, 2024

yaugenst-flex Jul 11, 2024

tylerflex Jul 11, 2024

tylerflex Jul 10, 2024

tylerflex Jul 10, 2024

yaugenst-flex Jul 11, 2024

tylerflex Jul 11, 2024

tylerflex Jul 10, 2024

tylerflex Jul 10, 2024

yaugenst-flex Jul 11, 2024

tylerflex Jul 11, 2024

tylerflex Jul 11, 2024

tylerflex Jul 10, 2024

tylerflex Jul 10, 2024

yaugenst-flex Jul 11, 2024

tylerflex Jul 11, 2024

yaugenst-flex commented Jul 11, 2024

tylerflex commented Jul 11, 2024

		@@ -17,6 +17,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

		### Changed

		### Changed



		class AbstractFilter(Tidy3dBaseModel, abc.ABC):
		"""A filter class for creating and applying convolution filters.

Rework make_filter interface to accept either size_px or radius and d… #1776

Are you sure you want to change the base?

Rework make_filter interface to accept either size_px or radius and d… #1776

Conversation

yaugenst-flex commented Jun 19, 2024

tylerflex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaugenst-flex commented Jun 19, 2024

tylerflex commented Jun 28, 2024

yaugenst-flex commented Jun 28, 2024

tylerflex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaugenst-flex commented Jul 11, 2024

tylerflex commented Jul 11, 2024