tardis
tardis copied to clipboard
Add Plotter class to plot_util module
This PR aims to solve the code overlap between SDECPlotter
and InteractionRadiusPlotter
. Here I add Plotter
class to plot_util.py
as a composition of plotter tools, which encapsulates some common functions used by both plotters.
Motivation and context
Cope with code overlap in pr #1606
How has this been tested?
- [ ] Testing pipeline.
- [x] Other.
Type of change
- [ ] Bug fix.
- [x] New feature.
- [ ] Breaking change.
- [ ] None of the above.
Checklist
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [ ] (optional) I have built the documentation on my fork following the instructions.
- [x] I have assigned and requested two reviewers for this pull request.
Codecov Report
Merging #1628 (3781fa3) into master (60cd602) will increase coverage by
0.00%
. The diff coverage isn/a
.
:exclamation: Current head 3781fa3 differs from pull request most recent head b2bb682. Consider uploading reports for the commit b2bb682 to get more accurate results
@@ Coverage Diff @@
## master #1628 +/- ##
=======================================
Coverage 61.87% 61.88%
=======================================
Files 63 63
Lines 5852 5861 +9
=======================================
+ Hits 3621 3627 +6
- Misses 2231 2234 +3
Impacted Files | Coverage Δ | |
---|---|---|
tardis/tardis/visualization/plot_util.py | 23.63% <0.00%> (-70.12%) |
:arrow_down: |
tardis/tardis/visualization/tools/sdec_plot.py | 11.19% <0.00%> (+1.24%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 60cd602...b2bb682. Read the comment docs.
Before a pull request is accepted, it must meet the following criteria:
- [ ] Is the necessary information provided?
- [ ] Is this a duplicate PR?
- [ ] If a new PR is clearly a duplicate, ask how this PR is different from the original PR?
- [ ] If this PR is about to be merged, close the original PR with a link to this new PR that solved the issue.
- [ ] Does it pass existing tests and are new tests provided if required?
- [ ] The test coverage should not decrease, and for new features should be close to 100%.
- [ ] Is the code tidy?
- [ ] No unnecessary print lines or code comments.
@yuyizheng1112, can you provide more information on the tests? Does making this change still behave as expected? I haven't run the code myself.
@jamesgillanders, I suppose it would go here or in the analysis folder. Since this doesn't really return anything, but is just used for plotting functions, it's probably fine to put here.
This needs rebasing
@jaladh-singhal I think it makes sense to do this in another PR because #1606 and this PR are editing different modules.
For the second question, do you mean if it is harder to write tests for SDEC Plot together with Plotter
class?
And I ran the SDEC plot in the notebook. The plot works as usual.
For the second question, do you mean if it is harder to write tests for SDEC Plot together with Plotter class?
Not exactly harder but we might need to change the way we're planning to write tests now.
Can you fix the merge conflict @yuyizheng1112 ? Then we can merge.
See #1838 as it pertains to this issue.
@visualization team - what is going on with this?
Closing this for now. The idea is sound, but we need to update the InteractionRadiusPlotter tool first