-
Notifications
You must be signed in to change notification settings - Fork 134
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 3D plotting option to Plotterly #932
Conversation
Code changes look good from reading them. I'll try using the new code soon |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #932 +/- ##
==========================================
+ Coverage 93.23% 93.40% +0.17%
==========================================
Files 200 200
Lines 12516 12538 +22
Branches 2580 2591 +11
==========================================
+ Hits 11669 11711 +42
+ Misses 595 585 -10
+ Partials 252 242 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
elif len(mapping) != self.dimension: | ||
raise TypeError("Plotter dimension is not same as the mapping dimension.") | ||
|
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.
The original logic allowed the number of mapping dimensions to be greater than the number of plotter dimensions. Both methods appear to be valid, and throwing an error rather than a warning is arguably more sensible. Nonetheless, I believe the following represents the original logic. Thought this was worth flagging, but feel free to mark as resolved.
elif len(mapping) != self.dimension: | |
raise TypeError("Plotter dimension is not same as the mapping dimension.") | |
elif len(mapping) < self.dimension: | |
raise TypeError("Plotter dimension is larger than the mapping dimension.") | |
elif len(mapping) > self.dimension: | |
warnings.warn("Mapping has length greater than plotter dimension. Additional mapping indices are ignored") | |
track_colors[track] = (self.fig.data[-1].line.color | ||
or self.fig.data[-1].marker.color | ||
or self.get_next_color()) | ||
|
||
if uncertainty: | ||
# earlier checking means this only applies to 2D. | ||
if uncertainty and self.dimension == 2: |
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.
For consistency, does it make more sense to move the checks for particle and uncertainty within the "dimension == 2" block / move the other dimensions checks out here?
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.
I was hesitant to do that because the dimension==2
line sits within a for-loop that goes through each track whereas uncertainty plotting is outside that. The current implementation plots all the tracks at once and then all the uncertainty at once. My worry is that changing the logic order to track, uncertainty for track, track2, uncertainty for track2, etc. may cause some unintended consequences. It's probably fine but I haven't tried it.
33cba73
to
6e8eadb
Compare
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.
All looks good to me. A good addition to the stone-soup plotting library.
You could change the plotting examples/MTT_3D_Platform.py to use Plotterly
instead of Plotter
.
This PR gives the option of plotting in 3D using Plotterly. It also adds more tests and makes dimension checking in Plotterly more holistic. More details are:
if self.dimension == 1: