-
Notifications
You must be signed in to change notification settings - Fork 3
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
Implement pol_convention
in pstokes
module
#404
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #404 +/- ##
=======================================
Coverage 96.09% 96.09%
=======================================
Files 17 17
Lines 6121 6152 +31
=======================================
+ Hits 5882 5912 +30
- Misses 239 240 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 @adeliegorce! A handful of comments, mostly related to documentation, as well as one substantial one about nsamples.
Hi @jsdillon, I made the requested changes. Let me know if you're happy with the new version. |
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 @adeliegorce -- I htink is almost ready to go, but does need a simple fix
Done! |
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 suggestion that will hopefully avoid confusion in some weird edge cases
Looks good, thanks @adeliegorce! |
This PR introduces some changes to the
pstokes
module for two purposespol_convention
attribute ofUVData
objects (see this PR) when constructing pseudo-Stokes polarizations whilst ensuring backwards compatibility (if there is nopol_convention
, then there is a warning andavg
is assumed). This is commit 6ffd0dc._combine_pol_arrays
(final name still open to debate) which takes lists of data, flags, and nsamples arrays and combines them into pseudo-Stokes following the appropriatepol_convention
(i.e.avg
andsum
). Although I see how useful this method can be, it does make the code slightly heavy so, reviewers, please let me know if you have any suggestions on how to make things cleaner. This is commit 2601670.I added new tests and modified existing ones accordingly.