tardis icon indicating copy to clipboard operation
tardis copied to clipboard

Add Plotter class to plot_util module

Open yuyizheng1112 opened this issue 3 years ago • 9 comments

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.

yuyizheng1112 avatar Jun 07 '21 16:06 yuyizheng1112

Codecov Report

Merging #1628 (3781fa3) into master (60cd602) will increase coverage by 0.00%. The diff coverage is n/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 Impacted file tree graph

@@           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.

codecov[bot] avatar Jun 07 '21 16:06 codecov[bot]

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.

tardis-bot avatar Jun 07 '21 17:06 tardis-bot

@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.

MarkMageeAstro avatar Jun 25 '21 13:06 MarkMageeAstro

This needs rebasing

andrewfullard avatar Jul 12 '21 13:07 andrewfullard

@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.

yuyizheng1112 avatar Aug 20 '21 11:08 yuyizheng1112

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.

jaladh-singhal avatar Sep 03 '21 15:09 jaladh-singhal

Can you fix the merge conflict @yuyizheng1112 ? Then we can merge.

andrewfullard avatar Dec 08 '21 16:12 andrewfullard

See #1838 as it pertains to this issue.

wkerzendorf avatar Dec 08 '21 16:12 wkerzendorf

@visualization team - what is going on with this?

wkerzendorf avatar Feb 23 '22 16:02 wkerzendorf

Closing this for now. The idea is sound, but we need to update the InteractionRadiusPlotter tool first

jamesgillanders avatar Nov 10 '23 15:11 jamesgillanders