Closed hdavid16 closed 2 months ago
Merging #186 (246eab6) into master (52f4aae) will increase coverage by
4.24%
. The diff coverage is54.22%
.
@@ Coverage Diff @@
## master #186 +/- ##
==========================================
+ Coverage 32.92% 37.16% +4.24%
==========================================
Files 9 9
Lines 565 635 +70
==========================================
+ Hits 186 236 +50
- Misses 379 399 +20
Impacted Files | Coverage Δ | |
---|---|---|
src/GraphPlot.jl | 100.00% <ø> (ø) |
|
src/lines.jl | 50.69% <47.82%> (-1.54%) |
:arrow_down: |
src/plot.jl | 57.57% <59.52%> (-5.07%) |
:arrow_down: |
src/layout.jl | 70.10% <100.00%> (+21.68%) |
:arrow_up: |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
I see thee are some errors on Julia 1.3. We probably should make the minimum version of this package v1.6.
I can review this when I find time, but in general I think it is better to keep different things in different PRs - makes it easier to review.
I see thee are some errors on Julia 1.3. We probably should make the minimum version of this package v1.6.
I have increased it to 1.6 and removed old compats. I also removed ColorTypes, which doesn't seem to be used anywhere.
@simonschoelly @etiennedeg Any updates?
Sorry about this. I'm not sure how it fix #150. I think changing for triangles is good, but maybe it would be better in another issue.
Simon's review is not addressed. For the remaining, I think we are good.
@etiennedeg, #150 is fixed by using arrows instead of angled lines because you can control the corner locations of the arrows. When you use angled lines to make an arrow, the actual tip of the arrow depends on the width of the lines.
Is the picture you are showing generated with the new triangle arrows? I can move the triangle arrows to a separate PR if desired where we can work it out further.
In terms of @simonschoelly 's comments, I have answered each of them and am awaiting for responses to my comments.
I'm fine breaking up these PR's if you and Simon prefer if that will truly help them get reviewed and merged faster. But if they'll still take months to review, I'm not sure of the benefit of having multiple PR's (at least from my point as a user, I want to be able to use these different features simultaneously while I wait for the PR to be reviewed). But I can resplit the PR into several smaller ones. Please confirm what you want me to do in this regard.
I think #150 is mostly an issue when saving to a non-square picture, because there is an inconsistency between absolute coordinates and relative coordinates which give strange distortions. IMO, the position of arrows in a square image was not an issue (at least not for the examples I tested). My picture is from the output of plots in Juno, and it is not generated as a square picture.
Another issue I see is when there is two arcs in opposite directions, it looks weird. Maybe we can fix this by starting the edge at the midway point between the two corners of the arrow tip?
Another issue I see is when there is two arcs in opposite directions, it looks weird. Maybe we can fix this by starting the edge at the midway point between the two corners of the arrow tip?
Great idea. I have added this fix in the last commit. I now check for if a reverse edge exists and then use the midway point on the arrow base for the line.
@simonschoelly do you want me to split this up into small PRs? This has been sitting here for several months. If I make small PRs would you or someone else review them? Thanks.
This is pending for too long and this would greatly improve the situation, we need to merge it. As it is now, it seems fine. @simonschoelly @gdalle, if you want to take a look, otherwise I will merge it in a week.
Realistically I won't have time to review this and I know nothing about GraphPlot so I'm leaving the decision in your hands. My approach is always that some improvement is better than no improvement at all, even if it's not perfect
@hdavid16 Thanks for the PR, and sorry for the excessive delay.
Thanks @etiennedeg . Might be good to go a new patch or minor release with the added features.
Yes, I wonder if we can get #193 in the release, but it will depend on Graphs#376. If it takes too long for someone to review it, I will do a release without it.
@hdavid16 I did a release
This PR merges the following individual PR's that address feature requests. I merged them to avoid conflicts when rebasing each time a different PR is merged.
178 - set background color (conflicts with changing plot size; conflict fixed in this PR). Closes issue #172.
181 - fix bug in shell_layout. Closes issue #149
182 - set plot size. Closes issues #94 #147 #161
183 - add plot title. Closes issue #107
184 - make self-loops curved. Closes issue #160
185 - fix bug in spring_layout. Closes issue #154
Also fixes the second point in issue #150 by switching the arrows to be triangles. Also closes issue #95 Also fixes issue #190