Skip to content
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

pre-release updates #42

Merged
merged 28 commits into from
Nov 6, 2023
Merged

pre-release updates #42

merged 28 commits into from
Nov 6, 2023

Conversation

s0larish
Copy link
Collaborator

@s0larish s0larish commented Oct 31, 2023

This patch adds @s0larish 's new contributions
-- @jmbhughes

Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Files Coverage Δ
solpolpy/constants.py 100.00% <ø> (ø)
solpolpy/instruments.py 100.00% <100.00%> (+4.54%) ⬆️
solpolpy/polarizers.py 96.86% <100.00%> (+6.68%) ⬆️
solpolpy/alpha.py 35.29% <0.00%> (ø)
solpolpy/core.py 77.02% <0.00%> (+11.50%) ⬆️

📢 Thoughts on this report? Let us know!.

@jmbhughes
Copy link
Member

Resolves #26

@jmbhughes jmbhughes self-requested a review October 31, 2023 17:32
Copy link
Member

@jmbhughes jmbhughes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great start! Let's get the CI passing too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't leave commented out code.

solpolpy/instruments.py Show resolved Hide resolved
@@ -19,6 +19,11 @@ def npol_to_mzp(input_cube):
in_list = list(input_cube)
conv_fact = (np.pi * u.radian) / (180 * u.degree)

if input_cube['angle_1'].meta['OBSRVTRY'] == 'STEREO_B':
offset_angle = -18 * u.degree * conv_fact # STEREOB
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These factors (-18, 45.8) are a bit cryptic to me. We should document them somehow more clearly. Where do they come from? What do they do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are obtained from Thompson (2015)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note this in the source code and the docstring fro this function.

Copy link
Member

@jmbhughes jmbhughes Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on this change for me? fourpol versus npol.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fourpol is for 0, 45, 90, 135 input angles
npol is for M', Z', P' which are separated by 60 degree but deviate from the standard MZP.

@jmbhughes jmbhughes changed the title updated config file pre-release updates Oct 31, 2023
Copy link
Member

@jmbhughes jmbhughes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed alpha.py has little test coverage. Please make sure it's tested.

The add_alpha function in core.py needs to be tested. It also needs #35 resolved.

Your fourpol_to_stokes method in polarizers.py is untested.

We could also improve the documentation everywhere, just docstrings. Don't worry about generating the docs website. We'll take care of that in another issue.

# return np.fliplr(np.arctan2(yy, xx))*u.radian
return np.flipud(np.rot90(np.fliplr(np.arctan2(yy, xx) + np.pi), k=1))* u.radian

return np.rot90(np.fliplr(np.arctan2(yy, xx)+np.pi), k=1)*u.radian
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not covered by tests. I notice most of the alpha.py file isn't. Let's write some tests and also make proper docstrings.

solpolpy/instruments.py Outdated Show resolved Hide resolved
solpolpy/polarizers.py Outdated Show resolved Hide resolved
solpolpy/polarizers.py Outdated Show resolved Hide resolved
data_out.append(("angle_3", NDCube(np.array([1]), wcs=wcs, meta={'POLAR': -60})))
data_out.append(("angle_1", NDCube(np.array([1]), wcs=wcs, meta={'POLAR': 60, 'OBSRVTRY': 'LASCO'})))
data_out.append(("angle_2", NDCube(np.array([1]), wcs=wcs, meta={'POLAR': 0, 'OBSRVTRY': 'LASCO'})))
data_out.append(("angle_3", NDCube(np.array([1]), wcs=wcs, meta={'POLAR': -60, 'OBSRVTRY': 'LASCO'})))
# data_out.append(("alpha", NDCube(np.array([0])*u.radian, wcs=wcs)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the commented out line.

Comment on lines 440 to 444
# """test pB from single angle fn in core"""
#
# output=pB_from_single_angle(B, B_theta, theta, alpha)
# np.testing.assert_allclose(output, expected, rtol=1e-05) No newline at end of file
# np.testing.assert_allclose(output, expected, rtol=1e-05)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be cleaned up.

@jmbhughes jmbhughes self-requested a review November 6, 2023 17:43
@jmbhughes jmbhughes merged commit 296557a into develop Nov 6, 2023
5 checks passed
@jmbhughes jmbhughes deleted the s0larish-patch-1 branch November 6, 2023 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants