-
Notifications
You must be signed in to change notification settings - Fork 4
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
Change PM plotter label, imports, calc report example #31
Conversation
Reviewer's Guide by SourceryThis PR makes three main types of changes: updates import statements to use absolute imports, adjusts the point plotter label positioning logic, and modifies the calculation report example parameters. Class diagram for updated import statementsclassDiagram
class Input
class get_capacity
class pmm_mesh
class point_plotter
class assign_max_min
class Column
class BarSize
class LoadCombination
class getCalculatedColumnProps
Input <.. get_capacity
Input <.. pmm_mesh
Input <.. point_plotter
Input <.. assign_max_min
Input <.. Column
Input <.. BarSize
Input <.. LoadCombination
Input <.. getCalculatedColumnProps
note for Input "Updated import paths to use absolute imports"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @janderson4 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
# label the intersections with the y-axis | ||
for i in (0, pt_count - 1): | ||
pos = (phi_Mn[i], phi_Pn[i]) | ||
label = str(round(phi_Pn[i], 1)) | ||
plt.plot(pos[0], pos[1], marker="+", ms=12, mew=1.2, c="black", zorder=3) | ||
plt.text(pos[0] + label_offsets[0], pos[1] + label_offsets[1], label, zorder=3) | ||
label_offset_y=label_offsets_y[0] if i==0 else label_offsets_y[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): The variable 'i' is undefined in this scope and could cause incorrect label positioning
This variable was only defined in the previous for-loop scope. Consider basing the y-offset on the point's position relative to the plot bounds instead.
.idea/workspace.xml
Outdated
@@ -0,0 +1,109 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's revert this file
I think we can remove the workspace file and then squash all the commits, then this is ready to merge the changes! |
from ...calc_document.calculation import calculation | ||
from ...calc_document.plotting import pmm_plotter_plotly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These imports need to remain relative in order to allow this package to operate outside of the efficalc repo. The files are still runnable with the instructions I left in the Readme
172eace
to
9e03122
Compare
Summary by Sourcery
Enhance the plotting functionality by adjusting label offsets for better positioning and update import paths in visual test scripts to use absolute paths for improved clarity.
Enhancements: