-
Couldn't load subscription status.
- Fork 65
Initial changes when working on PlaneWave, GaussianBeam, and ModeSource integration #118
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
Conversation
| """ | ||
| center = tuple((pt_min + pt_max / 2.0) for pt_min, pt_max in zip(rmin, rmax)) | ||
| size = tuple((pt_max - pt_max) for pt_min, pt_max in zip(rmin, rmax)) | ||
| center = tuple((pt_min + pt_max) / 2.0 for pt_min, pt_max in zip(rmin, rmax)) |
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.
nice catch on this 🤦
| # pylint:enable=line-too-long | ||
|
|
||
| version: str = '0.2.0' # will make this more automated later | ||
| version: str = "0.2.0" # will make this more automated later |
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.
What do you think of hardcoding the version number here, I think eventually we might want it somewhere else but this was a quick fix I made for the GUI guys
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.
Need to discuss separately, I don't understand all the implications and needs...
| """ | ||
|
|
||
| type: Literal["PlaneWave"] = "PlaneWave" | ||
| pol_angle: float = 0 |
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.
are we constraining pol_angle at all? to 0 -2pi or just letting it wrap?
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.
Doesn't matter on the backend so I guess no need to constrain?
| "z": plane_coords.z, | ||
| "f": np.array([self.freq]), | ||
| } | ||
| data_dict[field_name] = xr.DataArray(field, coords=coords) |
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.
so, did this end up working? seems to suffer from the 'single data point in one dimension' issue we discussed yesterday? or is it fixed?
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'm assuming we just need to make sure field has dimensions (Nx, Ny, Nz, Nf) and we should be good to go?
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.
Yes to your second comment. This currently passes tests. The modes part of the code is yet to change significantly, these are just some leftover changes from when I first looked at it some time ago. The issue yesterday was in another part of the code on the backend.
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 good, just want to clear up the things I commented about, otherwise we can merge if tests pass. Thanks!
No description provided.