Add field data to ModeMonitor (#1274) #1367

lucas-flexcompute · 2024-01-09T17:16:03Z

The implementation proposed here merges ModeSolverData and into ModeData. It also deprecates the use of ModeSolverMonitor and changes the implementation on ModeSolverData to be derived from ModeData (to avoid most code duplication and make deprecation later easier). There are 2 issues at this point:

ModeSolverData ended up inheriting the amps field, which will be unused.

This warning:

tidy3d/tidy3d/components/data/monitor_data.py

Lines 105 to 112 in fa40288

    
           @pd.validator("grid_expanded", always=True) 
        
           def warn_missing_grid_expanded(cls, val): 
        
               """If ``colocate`` not provided, set to true, but warn that behavior has changed.""" 
        
               if val is None: 
        
                   log.warning( 
        
                       "Monitor data requires 'grid_expanded' to be defined to compute values like " 
        
                       "flux, Poynting and dot product with other data." 
        
                   )

is being emitted (unnecessarily?) and getting caught in an adjoint test.

Thoughts, @tylerflex and @momchil-flex?

tylerflex · 2024-01-09T18:14:50Z

hm, let me look into this.

tylerflex · 2024-01-09T18:19:53Z

Alright, I see. Basically the adjoint test is using the emulated run function in tests/utils.py to get the ModeMonitorData for the test. In that emulated run function, the mode data constructor was missing a grid_expanded. I pass that in and the test works, I also formatted the test a bit better to use our new AssertLogLevel context manager and do string matching.

I'll make a PR to your branch

tylerflex · 2024-01-09T18:23:49Z

see https://github.com/flexcompute/tidy3d/pull/1368/files

lucas-flexcompute · 2024-01-09T18:27:37Z

Alright, I see. Basically the adjoint test is using the emulated run function in tests/utils.py to get the ModeMonitorData for the test. In that emulated run function, the mode data constructor was missing a grid_expanded. I pass that in and the test works, I also formatted the test a bit better to use our new AssertLogLevel context manager and do string matching.

I'll make a PR to your branch

Awesome, thanks! How do you feel about the overall implementation here, though? Should I keep going this direction and start on the backend changes?

tylerflex

looks good to me! just a few comments.

tidy3d/components/monitor.py

tylerflex · 2024-01-09T18:34:12Z

tidy3d/components/monitor.py

@@ -673,6 +693,17 @@ class ModeSolverMonitor(AbstractModeMonitor):
        "primal grid nodes).",
    )

+    @pydantic.root_validator(skip_on_failure=True)
+    def deprecation_warning(cls, values):


we should ensure that this change is reflected in our notebooks after this?

Yeah, but I think there's only one notebook that uses a ModeSolverMonitor so should be easy.

tidy3d/components/data/monitor_data.py

tylerflex · 2024-01-09T18:37:27Z

tidy3d/components/data/monitor_data.py

@@ -992,6 +1040,12 @@ def overlap_sort(
            modes is less than this threshold, a warning about a possible discontinuity is
            displayed.
        """
+        if len(self.field_components) == 0:
+            # raise DataError(


resolve comment? (remove or discuss?)

Yes, I feel the error would be more in line to other functions (like trying to calculate intensity without the fields present), but because we only return a copy of the original data with modes reordered, it makes sense to order an empty set of modes and not bother the user with checking if the fields exist before calling the method. So I prefer to return a copy, but I left the option to raise an error if that's preferable.

tylerflex · 2024-01-09T18:39:53Z

tidy3d/components/data/monitor_data.py

@@ -615,24 +622,29 @@ def _interpolated_tangential_fields(self, coords: ArrayFloat2D) -> Dict[str, Dat
        return fields

    def outer_dot(


if field_data is a ModeData then it's field components can now be None. do we explicitly make sure this is not the case anywhere when implementing these dot functions? (or other functions that use the fields?)

I think given that field_components still returns return {field_name: field for field_name, field in fields.items() if field is not None}, there should be no change in behavior

yea, I'm more worried about if the dot functions try to grab one of those components but it is missing, whether that is handled gracefully or you just get an internal KeyError

Yeah that's always been a possibility in FieldData, so it's been handled, you should just hit this: https://github.com/flexcompute/tidy3d/blob/1f22335e92d92cf875a1f70e4832acd9515a4c8b/tidy3d/components/data/monitor_data.py#L394C1-L395

That's right. All user-facing functions should handle missing components gracefully. Internal methods do not have those checks, for example: ModeData._isel, ModeData._find_ordering_one_freq, etc., but those are use-at-your-own-risk methods, so I guess the user should be responsible for the check.

momchil-flex · 2024-01-09T22:56:44Z

tidy3d/components/monitor.py

+        title="Propagation Direction",
+        description="Direction of propagation along the normal axis of the monitor used for the "
+        "computed mode field profiles.",
+    )


I understand why you've done this but I think it may be confusing to have this argument but still return mode amps in both directions. Like, someone reading the api docs may think that they have to set to "-" if they're doing mode decomposition in the backward direction. It's not terrible but it's a bit annoying to have this inconsistency (direction only affecting the mode fields but not the mode amps). However, I do agree that the other options that I see are also not great:

Do not provide direction and store fields in both directions, just like amps

Store amps only for the specified direction

So I don't know, maybe at least we should call it store_fields_direction? Or how about make store_fields a spec, store_fields = StoreFieldsSpec(direction="+")? We could imagine extending this in the future to e.g. only request a subset of frequencies, like just the central freq, etc.

Yes, I considered the same options when implementing this way but storing fields in both directions would be a major change and make ModeData and ModeSolverData interfaces incompatible.

I think your idea of using store_fields with a spec might be the better solution here. I'll go with that for now.

After implementing the StoreFieldsSpec I think we should reconsider. This looks a lot like over-engineering to me. Maybe using store_fields_direction makes more sense. I reverted the changes to this simpler implementation but left both in the commit history just in case.

Frequency and multiple direction selection are rare use cases, and can be accomplished by setting up multiple monitors. That way we keep the usual use case straightforward, but the advanced uses still possible.

Yeah, I agree with this evaluation.

momchil-flex

Looks good to me now!

tylerflex

looks good to me, sorry if you were waiting on me.

momchil-flex · 2024-01-26T00:40:12Z

So I looked through this again to see about any remaining parts of the code that may need to change. I think on the backend nothing much needs to change apart from replacing some ModeSolverMonitor instances in the tests with a ModeMonitor that stores modes. The only other thing I see is not in the backend per se but in the denormalizer: ModeData now has to optionally denormalize the fields as well, if present (I'll probably ask @dbochkov-flexcompute to take a look at that), and MC has to incorporate it in GUI eventually.

Well, that brings me to my main comment actually. ModeSolverData does serve a purpose that currently cannot be replaced (or doesn't make sense to be replaced) by a ModeData: namely, it is the return data structure of the ModeSolver. It doesn't make sense to be replaced by ModeData because that one currently always carries decomposition amplitudes, and optionally mode fields. Well, so there's technically two options:

Make amps in the ModeData optional, and make the ModeSolver return ModeData. I feel like this is confusing though.
Probably my preference: just don't deprecate ModeSolverMonitor. Keep it for its purpose as a monitor corresponding to a ModeSolverData, and maybe just print a warning in a Simulation if a ModeSolverMonitor is added that says that a ModeMonitor can also store fields?

lucas-flexcompute · 2024-01-26T02:06:29Z

Yes, I agree with the second option as well. I don't even think we need the warning. Just updating the tutorials and making it clear in the docs that a ModeSolverMonitor is no longer required should be enough to (slowly) move users to using a single monitor.

I'll finish any pending rebase issues tomorrow morning and commit a final version for merging front and backe-ends.

Signed-off-by: Lucas Heitzmann Gabrielli <lucas@flexcompute.com>

lucas-flexcompute · 2024-01-26T13:57:53Z

Since there's no need to remove ModeSolverDate, backend should be done. I've rebased and removed the deprecation warning.

tylerflex mentioned this pull request Jan 9, 2024

fix for lucas/fields_in_mode_data #1368

Merged

tylerflex reviewed Jan 9, 2024

View reviewed changes

momchil-flex reviewed Jan 9, 2024

View reviewed changes

momchil-flex approved these changes Jan 10, 2024

View reviewed changes

lucas-flexcompute marked this pull request as ready for review January 16, 2024 11:46

tylerflex self-requested a review January 25, 2024 16:53

tylerflex approved these changes Jan 25, 2024

View reviewed changes

tylerflex added the awaiting backend not to be merged as backend is not finalized label Jan 25, 2024

lucas-flexcompute force-pushed the lucas/fields_in_mode_data branch 2 times, most recently from 1ccad53 to 0548b5e Compare January 26, 2024 13:54

Add field data to ModeMonitor (#1274)

0548b5e

Signed-off-by: Lucas Heitzmann Gabrielli <lucas@flexcompute.com>

lucas-flexcompute merged commit 73aeb7a into pre/2.6 Jan 26, 2024
16 checks passed

lucas-flexcompute deleted the lucas/fields_in_mode_data branch January 26, 2024 14:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add field data to ModeMonitor (#1274) #1367

Add field data to ModeMonitor (#1274) #1367

lucas-flexcompute commented Jan 9, 2024

tylerflex commented Jan 9, 2024

tylerflex commented Jan 9, 2024

tylerflex commented Jan 9, 2024

lucas-flexcompute commented Jan 9, 2024

tylerflex left a comment

tylerflex Jan 9, 2024

momchil-flex Jan 9, 2024

tylerflex Jan 9, 2024

lucas-flexcompute Jan 10, 2024

tylerflex Jan 9, 2024

momchil-flex Jan 9, 2024

tylerflex Jan 9, 2024

momchil-flex Jan 9, 2024

lucas-flexcompute Jan 10, 2024

momchil-flex Jan 9, 2024

lucas-flexcompute Jan 10, 2024

lucas-flexcompute Jan 10, 2024

momchil-flex Jan 10, 2024

momchil-flex left a comment

tylerflex left a comment

momchil-flex commented Jan 26, 2024

lucas-flexcompute commented Jan 26, 2024

lucas-flexcompute commented Jan 26, 2024

	@pd.validator("grid_expanded", always=True)
	def warn_missing_grid_expanded(cls, val):
	"""If ``colocate`` not provided, set to true, but warn that behavior has changed."""
	if val is None:
	log.warning(
	"Monitor data requires 'grid_expanded' to be defined to compute values like "
	"flux, Poynting and dot product with other data."
	)

		@@ -615,24 +622,29 @@ def _interpolated_tangential_fields(self, coords: ArrayFloat2D) -> Dict[str, Dat
		return fields

		def outer_dot(

Add field data to ModeMonitor (#1274) #1367

Add field data to ModeMonitor (#1274) #1367

Conversation

lucas-flexcompute commented Jan 9, 2024

tylerflex commented Jan 9, 2024

tylerflex commented Jan 9, 2024

tylerflex commented Jan 9, 2024

lucas-flexcompute commented Jan 9, 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

momchil-flex left a comment

Choose a reason for hiding this comment

tylerflex left a comment

Choose a reason for hiding this comment

momchil-flex commented Jan 26, 2024

lucas-flexcompute commented Jan 26, 2024

lucas-flexcompute commented Jan 26, 2024