Skip to content

Conversation

@zhongjingjogy
Copy link

Try to plot with dashed lines.
test_lines96

@codecov
Copy link

codecov bot commented Jul 29, 2022

Codecov Report

Merging #148 (dcaeb78) into master (a400fe0) will increase coverage by 0.07%.
The diff coverage is 95.83%.

@@            Coverage Diff             @@
##           master     #148      +/-   ##
==========================================
+ Coverage   87.60%   87.67%   +0.07%     
==========================================
  Files          47       47              
  Lines        2146     2167      +21     
==========================================
+ Hits         1880     1900      +20     
- Misses        266      267       +1     
Impacted Files Coverage Δ
src/backend/BackendSVG.hpp 71.42% <ø> (ø)
src/util/Style.hpp 100.00% <ø> (ø)
src/util/Style.cpp 96.29% <90.90%> (-3.71%) ⬇️
src/backend/BackendSVG.cpp 73.52% <100.00%> (+0.19%) ⬆️
src/frontend/Axis.hpp 100.00% <100.00%> (ø)
src/frontend/Axis.tcc 95.55% <100.00%> (+0.20%) ⬆️
src/frontend/Line.tcc 68.00% <100.00%> (+0.25%) ⬆️

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 a400fe0...dcaeb78. Read the comment docs.

@zhongjingjogy
Copy link
Author

It seems that the last pull request was not merged. I wonder if the current pull request is acceptable or not. Looking for the positive response. Thanks!

Copy link
Collaborator

@martinjrobins martinjrobins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @zhongjingjogy. The previous PR was still in progress, so I'm not sure why you closed it! The new line styles look good, just a few things to tidy up see comments below

}

// get the number of maximum char length of yticks
int max_ytick_char_len() const { return m_max_ytick_char_len; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m_max_ytick_char_len depends on if the drawing code is run yet or not. please remove this method and keep this value purely private to the class

int max_ytick_char_len() const { return m_max_ytick_char_len; }

// get default font size
float font_size() const { return m_style.font_size(); }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this too as is not needed.

float font_size() const { return m_style.font_size(); }

// get maximum char
std::string max_ytick_char() const { return m_max_ytick_char; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and remove this


backend.text_align(ALIGN_RIGHT | ALIGN_MIDDLE);

size_t max_char_len = this->max_ytick_char_len();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
size_t max_char_len = this->max_ytick_char_len();
size_t max_char_len = 0;


if (strlen(buffer) > max_char_len) {
max_char_len = strlen(buffer);
m_max_ytick_char = std::string(buffer);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think m_max_ytick_char is not used? If so then remove

Vector<int, 2> get_ticks() const { return {m_nx_ticks, m_ny_ticks}; }

/// get the number of maximum yticks length (in pixels)
int max_ytick_pixels() const {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This calculation should really be done by the Style class, but since the calculation is still not correct please put it inline in Axis::draw_common_ylabel along with a TODO comment



TEST_CASE("ylabel position: case 1", "[axis]") {
std::vector<double> predicted_data = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename these vectors x and y and use these below rather than creating another pair of vectors

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants