-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduced axial ratio to DirectivityMonitor #1860
Conversation
0c66615
to
f1aef0b
Compare
I don't think we can rename it now, as it's released already in v2.7.0. But you can add a new monitor called RadiationMonitor, and add a warning in DirectivityMonitor that it'll be deprecated in v3.0. |
Yeah, sounds good. |
CHANGELOG.md
Outdated
- Introduce RF material library. Users can now export `rf_material_library` from `tidy3d.plugins.microwave`. | ||
- Users can now export `SimulationData` to MATLAB `.mat` files with the `to_mat_file` method. | ||
- Introduce RF material library. Users can now import `rf_material_library` from `tidy3d.plugins.microwave`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate RF material bullet points and possible mistake adding the MATLAB one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed "import" from "export" in line 14 and messed this up. I will revert these changes.
..., title="Axial ratio", description="Axial ratio with an angle-based projection grid." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe our convention is to capitalize each word in the title
@@ -1989,16 +1990,17 @@ class DirectivityData(MonitorData): | |||
|
|||
Example | |||
------- | |||
>>> from tidy3d import DirectivityDataArray | |||
>>> from tidy3d import DirectivityDataArray, AxialRatioDataArray | |||
>>> f = np.linspace(1e14, 2e14, 10) | |||
>>> r = np.atleast_1d(5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pedantic, but the radiation characteristics should be independent of r, since radiation intensity is scaled by r^2. Including r is not required, maybe if we rename later we can remove r, unless I am missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this projection distance r is used to compute the propagation factor to get the correct Etheta and Ephi.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right, but it ultimately isn't necessary for describing the resulting Directivity. Directivity only depends on frequency and angular position, the same as radiation intensity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just found that if we give up introducing the coordinate "r" here, we will need to do a corresponding coordinate change in the backend, which will make the code ugly. As this class inherits from FieldProjectionAngleMonitor, which has the coordinate "r", it's better to keep it consistent I guess? Users typically don't need to specify this value and can use the default value of 1e6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to keep it if the default value works fine in most simulations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, totally fine to keep it. I just wanted to point out that we don't absolutely need it in these DataArrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @QimingFlex, it looks pretty clear to me! I just found a couple of things to nitpick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All look nice!
Actually one comment: could you add the definition and the reference for the axial ratio? As you mentioned in the solver, the definition is not unique, so it will be good to clarify which definition is used. |
cfe3bc9
to
c394ceb
Compare
Yeah, I updated the reference to Balanis's equations (2-65) to (2-67). The backend computations are equivalent to these equations. Balanis uses absolute values and phase differences between two components for the computations. Instead, we compute using complex numbers directly. This has also been mentioned in the backend computations. |
Great! One minor format: the doc can read better if formatted in this way. Also I wonder if the reference should also be cited in the |
c394ceb
to
28a5192
Compare
Yeah, both have been updated, also updated drectivity reference in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just one changelog comment.
CHANGELOG.md
Outdated
@@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
- Added `WavePort` to the `TerminalComponentModeler` which is the suggested port type for exciting transmission lines. | |||
- Added plotting functionality to voltage and current path integrals in the `microwave` plugin. | |||
- Added convenience functions `from_terminal_positions` and `from_circular_path` to simplify setup of `VoltageIntegralAxisAligned` and `CustomCurrentIntegral2D`, respectively. | |||
- Added axial ratio computation to `DirectivityMonitor` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please mention how to grab this information, for example DirectivityMonitor.axial_ratio
. Also what is the axial ratio? consider adding , defined as the ratio of the major axis to the minor axis of the polarization ellipse
.
Adding this info into the changelog reduces the barrier of entry for people who just catch sight of the new feature to start using it.
otherwise PR looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, maybe the more technically correct thing here is to say that it is added to the "DirectivityData.axial_ratio
corresponding to a DirectivityMonitor
".?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rivised, thanks @tylerflex !
bd487fa
to
f8a5069
Compare
f8a5069
to
4042c5f
Compare
Introduced field axial_ratio in the DirectivityMonitor.
@dmarek-flex suggested renaming the DirectivityMonitor class to RadiationMonitor as radiation is more general and can include many characteristics such as directivity, gain, axial ratio, etc. I think this is reasonable and better. What do you think, @weiliangjin2021? Do you have other suggestions?