Fix to _C filename postfix and change to auto coloring
#262
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refines filename generation for single insets and updates the auto-coloring logic when only some regions are pre-colored:
- In
auto_color.cpp, it replaces the Dark2 palette with a single light gray for all uncolored regions and adds a shortcut for applying one color. - In
cartogram_info.cpp, it omits the inset-name postfix for single insets in SVG filenames. - The sample CSV data is updated to remove hard-coded colors for specified insets, reflecting the new auto-coloring behavior.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/inset_state/auto_color.cpp | Switched to a single light-gray palette and handled the single-color case |
| src/cartogram_info/cartogram_info.cpp | Conditionally include inset names only when multiple insets exist |
| sample_data/.../belgium_population_by_region_2022.csv | Removed explicit color entries for specified regions |
src/inset_state/auto_color.cpp
Outdated
| 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 " | ||
| "grey) to uncolored GeoDivs." |
There was a problem hiding this comment.
The log message implies partial coloring even when all regions may be pre-colored. Consider updating the condition to check for uncolored regions or adjusting the message to accurately reflect any preset colors.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
adisidev
left a comment
adisidev
left a comment
There was a problem hiding this comment.
You're almost there, but I think some more work is still required. Please have a look at the attached comments. Thank you for your work!
P.S. Before requesting review for this branch again, could you please merge Nihal's ci-improve branch? Thanks in advance.
| // 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); | ||
| } |
There was a problem hiding this comment.
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.
| // checking for color adjacency | ||
| if (palette.size() == 1) { | ||
| insert_color(gd.id(), palette[0]); | ||
| continue; |
There was a problem hiding this comment.
Although I know we used continue right before, it is usually bad practice.
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.
|
The performance comparison failed because in the current CI logic, PR from forks do not have permission write comment. Let me fix it soon. |
|
The fix should be online. Let's merge main again. |
3 participants
@adisidev
This pull request features a fix to one of the project's Github issues along with a change to how auto coloring works:
_Cpostfix #238: Removes the_Cpostfix from the filenames if there's only one inset