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

Allow length greater than 10m #11

Merged
merged 2 commits into from
May 4, 2020

Conversation

PedroBern
Copy link
Contributor

Fix #10

Copy link
Owner

@alfredocarella alfredocarella left a comment

Choose a reason for hiding this comment

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

Hi Pedro!

Thanks for taking the time for testing this package, and fixing a rookie mistake that slipped through the cracks. Your solution is very good, but I would like to suggest a small modification (only if you agree), and then I will merge it right away and update the package on pip.

I would like to preserve the upper limit of 10k points on the x-axis of the plots (which I think is an overkill already), so the code will not take extremely long in case someone picks a beam length of, say, 100m. So I propose changing line 307 in beambending/beam.py to something like:

x_vec = np.linspace(self._x0, self._x1, int(min((self.length) * 1000 + 1), 1e4))

I can take care of it if you are too busy, but I thought it would be nice to keep your contribution on the record since you took the time for sending it.

Regards,
-Alfredo

@PedroBern
Copy link
Contributor Author

Hi @alfredocarella, thanks for the kind words. Now I understand the reason of the limit. However, the suggestion you made is exactly what it was before, will limit to 10m. Could we keep at 100m? I can change to:

---  x_vec = np.linspace(self._x0, self._x1, int(min((self.length) * 1000 + 1), 1e4))
+++  x_vec = np.linspace(self._x0, self._x1, min(int((self.length) * 1000 + 1), 1e5))

Would it be ok? I agree that not having a limit is not ideal (as I initially proposed).

About the new example, is there something you would like me to change? The numbers were quite random.

@alfredocarella
Copy link
Owner

Sorry @PedroBern, I made a mistake with the parenthesis in my suggestion. Let me try to be more precise:

ORIGINAL CODE (with bug):
x_vec = np.linspace(self._x0, self._x1, min(int((self.length) * 1000 + 1), 1e4))
When length exceeds 10m, the value 1e4 (float) is picked as the number of points in the graph, which is the source of the original error (1e4 should have been properly cast to an integer)

MY FIRST (incorrect) PROPOSAL:
x_vec = np.linspace(self._x0, self._x1, int(min((self.length) * 1000 + 1), 1e4))
There is one misplaced parenthesis (plus one too many)

A POTENTIAL FIX:
x_vec = np.linspace(self._x0, self._x1, int(min(self.length * 1000 + 1, 1e4)))
Notice that int() and min() swap places compared to the original. When length exceeds 10m, the value 1e4 is cast to integer and picked as the number of points in the graph, thereby limiting an arbitrarily high number of points while lengths over 10m are allowed.
10k points should result in more than enough resolution for any weird curve IMO.

@PedroBern
Copy link
Contributor Author

I was misunderstanding the code. Now I see that x_vex is about the resolution, not the actual length! My bad. Thanks for explaining to me.

Ok, Already did the changes locally, it is passing the tests, but before I send the PR, about the example 4, would you like me to change something?

@alfredocarella
Copy link
Owner

I think example 4 is ok. You can leave it as is, or remove it if you don't believe it adds much information on top of example 1. I can accept both options, so I will leave it up to you.

@alfredocarella alfredocarella merged commit 5bc0536 into alfredocarella:master May 4, 2020
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.

Can't run example with Beam equal or greater than 10m
2 participants