-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add _repr_svg_() #111
base: main
Are you sure you want to change the base?
Add _repr_svg_() #111
Conversation
Quality checks have failed. I'll fix this and do another PR. |
No need to do another PR! You can keep pushing commits to the same branch until the CI goes green |
Thanks for letting me know. Still getting to grips with this!
…On Sun, 21 May 2023, 00:57 Juan Luis Cano Rodríguez, < ***@***.***> wrote:
No need to do another PR! You can keep pushing commits to the same branch
until the CI goes green
—
Reply to this email directly, view it on GitHub
<#111 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AO4AFMC2E4WWHFYGWDJQKPDXHE46JANCNFSM6AAAAAAYI6UBME>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #111 +/- ##
==========================================
- Coverage 99.22% 95.24% -3.99%
==========================================
Files 12 12
Lines 773 967 +194
==========================================
+ Hits 767 921 +154
- Misses 6 46 +40
☔ View full report in Codecov by Sentry. |
This patch fixes:
|
…packet_svg_with_point()
This push increases code coverage. |
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.
Thanks a lot for contributing this @Stoops-ML !
There's one thing that irks me though, and it's all the canvas size and coordinates mangling that's happening, both in the _svg
method of the objects and the _repr_svg_
method itself. I'm wondering if we could standardize on a given size (400 x 300 or anything else that is reasonable) and use relative coordinates for everything to dramatically simplify all that. Otherwise it will be very difficult to make sure that all the logic is correct.
What do you think?
What do you mean exactly by canvas size and coordinates mangling? Sorry for my ignorance, I'm new to this so please be patient with me! The approach I followed is based on what Shapely does. See here for how they make a point, and here for how they combine several points (or anything else) together. To clarify so that we're on the same page: the canvas size depends on where the points on the canvas are. The closer the points the smaller the canvas size desired (as long as it's bigger than the minimum canvas size). This results in closer points on a canvas looking bigger on the screen, and farther away points on a canvas looking smaller. The main issue I have with calculating relative coordinates is that it will add a lot of complexity to the code in the Packet() class that will replace the simple bounds calculation. And after all that work the created SVG would be the same between the two implementations. I'm not sure what mangling exactly means so I may be wrong. |
I've done some refactoring, so hopefully things are a bit more clear now. Also, any tips or feedback would be greatly appreciated! |
I see! If Shapely already has this logic, wouldn't it be easier if we
That way, we would avoid having to maintain lots of code, and benefit from any improvements done in Shapely. In fact, this can be more widely useful: if we were able to transform czml3 entities to GeoJSON, I bet many people would benefit from that (see for example gh-83). Then the conversion would be czml3 -> GeoJSON -> Shapely -> SVG. I appreciate this contribution a lot and I'm sure you put lots of hours into it, which I'm grateful for. However, I think we should rework it. In its current form, I have two options:
On the other hand, if we find another way of achieving the same by leveraging the Python ecosystem:
Hope this doesn't come across as blunt or ungrateful! |
I personally don't like the idea of using Shapely for the following reasons:
Regarding your point of leveraging the Python ecosystem, i.e. Shapely:
Finally, I understand that you don't have much time to review code. However, no matter what the PR is you'll have to review it. There's definitely a trade-off between accepting contributions and maintaining package stability and maintainability, and I'm sure you'll strike the right balance that's in the best interest of the community. |
95.24% coverage. |
… when calling _repr_svg_()
An SVG image is displayed for BaseCZMLObject() objects that have Position() or PositionList() attributes (if defined). The Colour() attribute is used if defined, else black is used.
No image is displayed for BaseCZMLObject() objects that don't have Position() or PositionList() attributes, nor for BaseCZMLObject() objects that don't have Position() or PositionList() attributes defined.
This is supported for:
Note that the Packet() class combines all SVGs that it contains, and supports Point() and Path() classes which have their positions defined externally (i.e. within the packet and not within themselves).
An example in Jupyter lab: