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

create workflow ci #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

create workflow ci #6

wants to merge 1 commit into from

Conversation

samgawthrop
Copy link

testing a workflow ci

@samgawthrop samgawthrop marked this pull request as ready for review March 24, 2020 20:40
@samgawthrop
Copy link
Author

you ok if I do a squash-and-merge, or should I crush these down to a single commit first?

testing a workflow ci

reverting/patching

lot the plot: exception

pep 8 ing

Trying out badge

Modified rules, refactored code accordingly

Fixing test import paths

checking in on imports

adding a touch function to use cython imports, horribly

had a bad assertion
Comment on lines +14 to +19
def touch():
"""Touch all imports from cython to alleviate flake8."""
if valenti_q_acc == valenti_q_mag:
print(rotation_vector_to_matrix)
identity(valenti_q_mag)
raise Exception("Never run this")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not clear on what this does.

Copy link
Author

Choose a reason for hiding this comment

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

This is an abomination to use the things imported from the _pyquat. I regret it already, but I'm not sure how to do this any better.

Unused imports are bad, but the unused imports here are kinda-sorta necessary to load the context for the other things downrange that use this init as the conduit into the cython.

Cython itself didn't have any particularly clear patterns on how to explicitly import without leaving unused imports, or having unclear context.

else:
raise InputError("expected 1- or 2-D array")
raise Exception("expected 1- or 2-D array")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be a ValueError.

Copy link
Author

Choose a reason for hiding this comment

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

fair I couldn't find a deceleration for InputError. I'll change these. I too dislike base Exceptions.

Comment on lines -56 to +57
for i in range(1,ary.shape[1]):
axes.plot(ary[0,i-1:i+1], ary[1,i-1:i+1], ary[2,i-1:i+1], c = tuple(c[:,i]), **kwargs)
for i in range(1, ary.shape[1]):
axes.plot(ary[0, i - 1:i + 1],
ary[1, i - 1:i + 1],
ary[2, i - 1:i + 1],
c=tuple(c[:, i]), **kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't love this type of change. It's going to make some code absolutely unreadable. I realize that expressions should be space-separated in general, but these are expressions within bracket notation.

Comment on lines -73 to +80
xh = numpy.zeros((3,2))
xh = numpy.zeros((3, 2))
yh = numpy.zeros_like(xh)
zh = numpy.zeros_like(xh)
xh[0,1] = axis_size
yh[1,1] = axis_size
zh[2,1] = axis_size
xh[0, 1] = axis_size
yh[1, 1] = axis_size
zh[2, 1] = axis_size
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as with the array indexing. I don't think this change substantially improves readability.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, I'm going to push back on the second part of this one a bit more because it doesn't actually appear in pep8: https://stackoverflow.com/questions/31741418/pep8-space-after-a-comma/37064013#37064013

I think there should be spaces after commas except within array notation. The examples in pep8 do show spaces after commas, even though this isn't specifically enumerated; but they don't show multi-dimensional array indexing notation.

Thoughts?

@@ -55,11 +54,9 @@ def rand(

"""
if axis is not None and angle is not None:
raise StandardError("expected non-fixed angle or non-fixed axis or both")
raise Exception("expected non-fixed angle or non-fixed axis or both")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
raise Exception("expected non-fixed angle or non-fixed axis or both")
raise ValueError("expected non-fixed angle or non-fixed axis or both")

test/test_pyquat.py Show resolved Hide resolved
@@ -58,7 +60,6 @@ def test_davenport_eigenvalues(self):
[1.0, 0.0],
[0.0, 1.0]])
B = pq_esoq.attitude_profile_matrix(obs = obs, ref = ref)
irot = pq_esoq.sequential_rotation(B)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line is needed. It does an in-place rotation on B.

Copy link
Author

Choose a reason for hiding this comment

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

so it should just be pq_esoq.sequential_rotation(B)? the irot variable isn't used. Also curious why it passes tests with an essential rotation removed, but that's a differnt question.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest an additional line that verifies that irot is not None. self.assertNotEqual(irot, None) maybe?

Comment on lines -22 to +18
q = pqv.dq_acc(a0)
q = pqv.q_acc(a0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIRC, these are different functions.

Copy link
Author

Choose a reason for hiding this comment

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

where's dq_acc declared? I see valenti_q_acc imported through the base init , which is laundered through wahabi.valenti to be q_acc.

from pyquat import valenti_q_mag as q_mag
from pyquat import valenti_q_acc as q_acc

but I don't see dq used elsewhere in the code, is that renamed elsewhere in an import?

these are getting hard to follow.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we settled this during our zoom meeting yesterday. Two additional import lines should be added (in addition to the two imports you gave in your comment):

from pyquat import valenti_dq_mag as dq_mag
from pyquat import valenti_dq_acc as dq_acc

and the above change should be reverted.

Comment on lines -39 to +35
dqm = pqv.dq_mag(l)
dqm = pqv.q_mag(l)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also different functions.

@@ -47,7 +43,7 @@ def test_q_mag_points_north(self):
self.assertEqual(dqm.y, 0.0)

q_G_to_L = qa * qm
q_L_to_G = q_G_to_L.conjugated()
# q_L_to_G = q_G_to_L.conjugated()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# q_L_to_G = q_G_to_L.conjugated()

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.

None yet

2 participants