Skip to content

Menno's updated branch pull request - #48

Open
MennoLaveaux wants to merge 4 commits into
jatkinson1000:mainfrom
MennoLaveaux:menno-branch
Open

Menno's updated branch pull request#48
MennoLaveaux wants to merge 4 commits into
jatkinson1000:mainfrom
MennoLaveaux:menno-branch

Conversation

@MennoLaveaux

Copy link
Copy Markdown

This pull request adds the solution to Issue #37 .

Cool stuff!

Added the GNU General Public License version 3 to the project.
Add a comprehensive .gitignore file for Python projects

@jatkinson1000 jatkinson1000 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks @MennoLaveaux This is a great start to the PR, linking to the issue you opened.

I have made a "suggestion" in my code review that you can try merging into your code using the GitHub Pull Request interface.

I also see we have all of the other files from the workshop creeping in.
Ideally these would be on other branches so that only the new equation is being contributed here. The intermediate git course would give you some clues on how to clean this up ;)

Calculate pendulum length from the period.
"""
return m * 9.81 * max_height(len, theta)
return 9.81 * period**2 /(4 * np.pi**2)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This might be a bit clearer and more concise:

Suggested change
return 9.81 * period**2 /(4 * np.pi**2)
return 9.81 * (period / (2.0 * np.pi)) ** 2.0

-------
float
energy [kg . m2 /s2] of the pendulum
Calculate pendulum length from the period.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Perhaps we could extend the docstring here to conform to numpy style?

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