New path tree#1048
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1048 +/- ##
=======================================
Coverage 81.28% 81.28%
=======================================
Files 142 142
Lines 15643 15643
=======================================
Hits 12716 12716
Misses 2927 2927 ☔ View full report in Codecov by Sentry. |
I believe the most I have ever done with the tree representations was this least changed path notebook (It is old enough to have the wrong colors xD) (for context it was mainly used to check that all the right frames were selected) |
… into new_path_tree
This will need some cleanup, but it is in reasonable shape now
… into new_path_tree
|
I'm going to mark this as ready for review, because this needs to be in the 1.7 release (due out this weekend). The policy on "experimental" modules is that we don't require unit tests in place (even though it pains me not to have them). I've given this a decent bit of testing via a Jupyter notebook, which I tossed into a gist: https://gist.github.com/dwhswenson/4755b710941844e3aff488cbbe512359 The API is not completely solidified yet, especially around how to configure more custom coloring options. However, I'm reasonably confident in the parts of the API the separate the content of the path tree from its presentation as SVG, Matplotlib, or other front-end. Things I might try to add before merge:
Things we'd like to add in the future:
Given the lack of tests and reasonably large code base, I'm going to leave this open for review as long as I can, while still getting 1.7 released. This is ready for review and comment. Without review, I will merge this after at least 80 hours, no sooner than Fri 30 Aug 17:00 GMT (19:00 my local). |
| block.append(block, maximum - minimum) | ||
| minumum = 0 | ||
| maximum = len(step.trajectory) | ||
| block = [] |
There was a problem hiding this comment.
block is a list, so the last line of this block cancels the first one?
There was a problem hiding this comment.
looking into this further; current code is working, but this makes me confused as to why (maybe this code isn't hit?) Is looks to me like that should be blocks.append(...), which would also make resetting the block (local variable) make sense.
There was a problem hiding this comment.
cleaned it up a bit, but this code isn't used yet (which explains why it wasn't breaking anything)
Co-authored-by: Sander Roet <sanderroet@hotmail.com>
Co-authored-by: Sander Roet <sanderroet@hotmail.com>
|
@sroet : This can take another pass, if you have time. I think I addressed all the changes you requested. Without review, I will merge this after at least 8 hours, no sooner than Sat 31 Aug 21:00 GMT (23:00 my local). |
sroet
left a comment
There was a problem hiding this comment.
One leftover doc that points to a function that doesn't seem to exist, LGTM otherwise!
This PR starts on a new path tree code. The main reason for this is that the old path trees do not fully work with SimStore because they rely on a bad design decision that SimStore is removing. Additionally, the old path trees don't work with two-way shooting in either SimStore or netcdfplus.
Furthermore, there is no unit testing for the old path trees, and they're written in high complexity code that would be difficult to test. There's also very little documentation to help anyone else maintain that code.
Plus, I think the new version has a much nicer API, especially for the most common use case (selecting replica 0):
Also, the API for added new plotting utilities is very straightforward: you create a class that implements two functions:
draw_trajectory(row, step, options)draw_connector(x, bottom, top, step, options)In these
stepis a namedtuple that carries themover, thetrajectory, the boolaccepted, andsegments, which provides information about which parts of the trajectory were new and which existed in the prior trajectory.optionsus the object which carries all the user-customizable plotting options, also contains a function that will get the left and right endpoints of the trajectory.rowis the row number of the trajectory.xis the location of the shooting point, and bottom and top are row numbers to connect the trajectory to the shooting point origin. Examples of this are in thempl.pyfile.Unfortunately, there's a lot of really good functionality in the old path trees that will take time to replicate. I'm curious if anyone has ever actually used some of those (like changing the CSS to use different colors, especially colors that vary across a trajectory, representing the values of the snapshot, or adding text labels per snapshot).
2024 UPDATE: Overview of the contents here:
path_tree.py: Generate the objects that describe the path tree. This is content, as separated from presentation, but has conveniences for a few presentation modes.options.py: Attempt to generalize presentation options in a way that might be valid across multiple presentation tools. This is the part of the API that is most likely to undergo massive changes.plotter.py: ABC for presentation-specific plotting classes. This may see some future refactoring.mpl.py: Tools for using matplotlib for presentation.svg.py: Tools for using SVG for presentation.The use modes that are expected to be supported are:
Use conveniences in the
PathTreeobject to plot:Explicitly do content and presentation in separate steps: