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

Results arrays (#144) #145

Merged
merged 3 commits into from
Feb 1, 2023
Merged

Results arrays (#144) #145

merged 3 commits into from
Feb 1, 2023

Conversation

connorferster
Copy link
Collaborator

@connorferster connorferster commented Jan 31, 2023

Addresses #144 with the following changes:

  • Added an "array" method for Member3D for the following:
    • shear_array
    • moment_array
    • torque_array
    • axial_array
    • deflection_array
    • rel_deflection_array
  • Added an n_points argument to the "plot" methods to allow for varying the resolution of the plot (added the argument as the final keyword argument to not break any existing code).
  • Added an example for the use case of plotting factored envelopes of varying number of points (Examples/"Simple Beam - Factored Envelope.py")
  • Fixed bug in Member3D.rel_deflection where the actual LoadCombo object was being passed to Member3D.d instead of the combo name.
  • Fixed various typos in doc strings of "plot" methods

…ys for shear, moment, torque, axial, deflection, and relative deflection. Passes current tests. Addresses #114.
… caused an error when calling Member3D.rel_deflection
@connorferster connorferster changed the title Results arrays (#114) Results arrays (#144) Jan 31, 2023
@connorferster
Copy link
Collaborator Author

@JWock82

Let me know your thoughts on this and if you would like any other additions to the code base to support this proposed change.

@JWock82
Copy link
Owner

JWock82 commented Jan 31, 2023

I like this. It seems like shear_array and plot_shear are similar to each other and this could be done in plot_shear with an argument that switches the output from matplotlib to an array. If you want to merge the methods together feel free and then issue another PR. Otherwise I'll merge this PR and then merge the methods myself when I find a spare moment in the next week or so.

@connorferster
Copy link
Collaborator Author

connorferster commented Jan 31, 2023

Glad you like it!

Agreed, the "array" functions and the "plot" methods are similar and the implementation code is repetitive. I hear what you are saying on adding a boolean flag to the plot methods. My only misgiving with such an approach is that it would make the return type of the plot functions inconsistent which I think is unexpected. If (I mean "when") I was cruising the API to see what method to use to get a result array, the plot methods would not be my first guess.

However, I think that reducing the amount of repetition would be good because it reduces the quantity of code requiring maintenance. Let me know what you think of this counter proposal:

Add a method like this:

def _results_array(self, action: str, n_points: int, direction: Optional[str] = None, combo_name="Combo 1") -> np.ndarray:
    """
    Returns the results array according to the parameters.

    action: {"shear", "moment", "torque", "axial", "deflection", "rel_deflection"}
    n_points: the number of points
    direction: the direction of action to retrieve
    combo_name: the load combination to retrieve
    """
    action_function = getattr(self, action)
    L = self.L()
    x_arr = linspace(0, L, n_points)
    if direction is None:
            y_arr = array(
                [self.action_function(x, combo_name) for x in x_arr]
            )
    else:
            y_arr = array(
                [self.action_function(direction, x, combo_name) for x in x_arr]
            )
    return array([x_arr, y_arr])

Then, the implementation of each array function simply becomes:

def shear_array(direction: str, n_points: int, combo_name="Combo1") -> array:
    """
    ...doc string
    """
    return self._results_array("shear", direction, n_points, combo_name)

The plot functions could then call the _results_array methods in a similar fashion. This would leave, effectively, only one method that requires maintenance. If you wanted to take it further, you could do the same with the plot methods: abstract it into a single _results_plot method (for example) and then each plot method would simply call that, each with the appropriate inputs. Then you would only have two methods that need maintaining and you have created convenient and intuitive API endpoints for the user (i.e. plot_shear and shear_array, etc.)

Since these methods would be "private" methods, and only called internally by other methods, you could also do away with a lot of error checking (like checking if the action string passed was a valid action name). The current error checking in all of the plot functions could be moved to the _results_array method, further reducing the maintenance surface.

Let me know your thoughts on this and I can put it together.

@JWock82 JWock82 merged commit fbfd1da into JWock82:master Feb 1, 2023
@JWock82
Copy link
Owner

JWock82 commented Feb 1, 2023

I see what you're getting at, and I'll probably move it that direction. I've merged your PR for now.

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