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

Utils test and winding order algorithm #198

Merged
merged 2 commits into from Mar 14, 2020

Conversation

lidonohu
Copy link
Contributor

Closing previous PR, all changes made and redone.

@pep8speaks
Copy link

Hello @lidonohu! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 930:80: E501 line too long (80 > 79 characters)

@codecov
Copy link

codecov bot commented Mar 12, 2020

Codecov Report

Merging #198 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #198      +/-   ##
==========================================
+ Coverage   88.37%   88.45%   +0.07%     
==========================================
  Files          17       17              
  Lines        4234     4262      +28     
  Branches      529      532       +3     
==========================================
+ Hits         3742     3770      +28     
  Misses        354      354              
  Partials      138      138
Impacted Files Coverage Δ
fury/utils.py 89.54% <100%> (+0.89%) ⬆️

Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Hi @lidonohu,

Your tests are really nice! good job! Thank you for this new functionality too!
Below, I made some comments but I think, they are quick and easy to fix.
Thank you

def what_order(verts, tri):
"""Determines the winding order of a given set of vertices and a triangle

Parameter
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: Parameters


npt.assert_equal(test_tri[0], utils.change_order(test_tri[1]))

npt.assert_equal(test_tri[2], utils.change_order(test_tri[3]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice tests!


Returns
-------
0 or 1: int
Copy link
Contributor

Choose a reason for hiding this comment

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

order_type : bool instead

def change_order(tri):
""" Changes the order of a given triangle

Parameter
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above


Returns
-------
np.array(newVert): ndarray
Copy link
Contributor

Choose a reason for hiding this comment

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

new_verts : array instead



def check_order(vert, triarr):
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

missing some information

def check_order(vert, triarr):
"""

Parameter
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above

@skoudoro skoudoro added this to the v0.5.0 milestone Mar 12, 2020
@skoudoro
Copy link
Contributor

Thank you really much for this work @lidonohu! merging

@skoudoro skoudoro merged commit 7d162c5 into fury-gl:master Mar 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants