Skip to content

Fix to _C filename postfix and change to auto coloring #262

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.

Sign up for GitHub

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

Closed
SaifKhan21 wants to merge 6 commits into from
Closed

Fix to _C filename postfix and change to auto coloring #262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a09bcc1
Change to color all uncolored regions to gray if only some have colors
SaifKhan21 May 27, 2025
e95d26b
Fix to _C file postfix when there's only one inset
SaifKhan21 May 27, 2025
861690d
Merge branch 'main' of https://github.com/SaifKhan21/cartogram-cpp
SaifKhan21 May 28, 2025
b9f6204
Apply suggestions from code review
adisidev May 29, 2025
ce96ef9
Merge branch 'mgastner:main' into main
SaifKhan21 May 29, 2025
26d60c0
Merge branch 'mgastner:main' into main
SaifKhan21 Jun 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions sample_data/belgium_by_region_since_1995/belgium_population_by_region_2022.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
shapeName,Population (people),Color,Inset,Label
Brussels-Capital,1222637,#fbb4ae,,
Flanders,6698876,#b3cde3,,
Brussels-Capital,1222637,,,
Flanders,6698876,,,
Wallonia,3662495,#ccebc5,,
11 changes: 8 additions & 3 deletions src/cartogram_info/cartogram_info.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,12 @@ void CartogramInfo::write_svg(const std::string &suffix)
for (const InsetState &inset_state : inset_states_) {
inset_names += inset_state.pos();
}
insets_combined.write_map(
map_name_ + "_" + inset_names + "_" + suffix,
false);
// Only attach inset names to filename if there's more than one inset
if (n_insets() > 1) {
insets_combined.write_map(
map_name_ + "_" + inset_names + "_" + suffix,
false);
} else {
insets_combined.write_map(map_name_ + "_" + suffix, false);
}
Comment on lines +451 to +458
Copy link
Collaborator

@adisidev adisidev May 29, 2025

Choose a reason for hiding this comment

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

Could you double check what happens without this commit?

I'm worried that we are writing the same figure files twice: once from the inset_state loop and once here. In #238, I mentioned the following files are produced:

belgium_population_by_region_2022_C_cartogram.svg  
belgium_population_by_region_2022_C_input.svg 
belgium_population_by_region_2022_input.svg 
belgium_population_by_region_2022_output.svg
belgium_population_by_region_2022_cartogram.geojson

I need you to check whether _C_input is the exact same as _input (likewise for _C_output and _output). If they are the same, then we should not just change the output name for _C_input but not write _C_input at all. On the other hand, if they are different, we'd need to ensure they still have different names so that they are both written, and nothing is overwritten due to having the same name.

}
23 changes: 12 additions & 11 deletions src/inset_state/auto_color.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,12 @@ void InsetState::auto_color()
std::vector<Color> palette;

if (colors_.size() > 0) {
// If some colors are already provided, use a different palette
// From https://colorbrewer2.org/#type=qualitative&scheme=Dark2&n=8
std::cerr << "Some but not all colors provided, using Dark color palette."
// If some but not all of the GeoDivs are colored, use only #f2f2f2 (light
// gray) to color the rest
std::cerr << "Some but not all colors provided, assigning #f2f2f2 (light "
"gray) to uncolored GeoDivs."
<< std::endl;
palette.emplace_back("#1b9e77"); // aqua green
palette.emplace_back("#d95f02"); // dark orange
palette.emplace_back("#7570b3"); // purple
palette.emplace_back("#e7298a"); // dark pink
palette.emplace_back("#66a61e"); // olive green
palette.emplace_back("#e6ab02"); // dark yellow
palette.emplace_back("#a6761d"); // brown
palette.emplace_back("#666666"); // dark grey
palette.emplace_back("#f2f2f2"); // light gray
} else {
// Using default palette for now
// TODO: Accept palette from user
Expand All @@ -48,6 +42,13 @@ void InsetState::auto_color()
if (color_found(gd.id()))
continue;

// If there's only one color in palette, color the div with it without
// checking for color adjacency
if (palette.size() == 1) {
insert_color(gd.id(), palette[0]);
continue;
Copy link
Collaborator

@adisidev adisidev May 29, 2025

Choose a reason for hiding this comment

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

Although I know we used continue right before, it is usually bad practice.

https://softwareengineering.stackexchange.com/questions/58237/are-break-and-continue-bad-programming-practices

Here is some more discussion on the topic. Generally, if continue is used as a precondition, it's still okay, which is why the earlier continue gets a pass. However, multiple continues can make code confusing.

I'd suggest adding the following code snippet below line 19:

    for (GeoDiv &gd : geo_divs_) {
      if (!color_found(gd.id())) {
        insert_color(gd.id(), Color("#f2f2f2"));
      }
    }
    return;

This way, the contiguity_graph does not created just yet (as it is not needed), and the code is more understandable.

}

for (unsigned int i = 0; i < palette.size(); ++i) {
const Color c = palette[i];
bool shared_color = false;
Expand Down
Loading