add option for frame to rotate modes internally if required #226
Conversation
|
@vijayvarma392 this PR is ready to review now. As described at the top, it adds an option to rotate modes internally. |
|
@vijayvarma392 let me know if you have any other comments on this. |
|
I have addressed all the comments @vijayvarma392 |
| # frame, rotate the modes and obtain the corresponding modes in the | ||
| # coprecessing frame | ||
| if self.precessing is True and self.frame == "inertial": | ||
| warnings.warn( |
There was a problem hiding this comment.
To avoid annoyance, shouldn't you be using debug level? Same comment for all warnings.
There was a problem hiding this comment.
yes, that came to my mind as well. However, the issue was that this was happening while processing the dataDict before setting values to debug_level. The debug_level is available via extra_kwargs which we set only after processing the dataDict.
There was a problem hiding this comment.
I see, was there a good reason for doing it like that? Would it make sense to flip things around, or even just read or pass extra_kwargs['debug_level'] where needed?
There was a problem hiding this comment.
Will the following work? Inside process_dataDict we check if debug_level is provided by the user via extra_kwargs and use it if its there else use the default value 0 as in the default_extra_kwargs
There was a problem hiding this comment.
The reason was that we needed information, for example, the width for peak finding, from the process_dataDict and then only set the default values.
There was a problem hiding this comment.
But if extra_kwargs is not provided then it's None which will raise an error.
There was a problem hiding this comment.
No, I mean, what is the entire debug_level option is made independent of extra_kwargs, like we did for https://github.com/vijayvarma392/gw_eccentricity/blob/main/gw_eccentricity/eccDefinition.py#L144. I don't know if this is the right approach, asking.
There was a problem hiding this comment.
Yes, that will solve this issue. Will need to make changes at several places though so that the checks pass. Would it be better to make that change in a separate PR?
There was a problem hiding this comment.
Sounds good. Revert the last commit first?
|
Is it |
Better is |
This reverts commit f83d7e7.
Adds an argument
framewhich takes either of the two valuesinertialorcoprecessing. If the system isprecessingandframe='inertial', the modes are rotated internally to get the dataDict in the coprecessing frame. This completes the extension ofgw_eccentricityto precessing system.