Fix Issues with Shifting of Insets to Position and String to Decimal Converter #218

nihalzp · 2024-11-17T19:19:36Z

Closes #219
Closes #214
Closes #204
Closes #180

adisidev

I think there are still a few changes needed. Let me try and make those, but it would be good for you @nihalzp to still read my code comments so we can keep our programming standards the same and avoid future review comments.

include/cartogram_info.hpp

include/inset_state.hpp

adisidev · 2024-11-28T03:05:33Z

src/inset_state/check_topology.cpp

-            std::cerr << "Hole: " << h << std::endl;
-            std::cerr << "Polygon: " << ext_ring << std::endl;
-            std::cerr << "GeoDiv: " << gd.id() << std::endl;
+            std::cerr << "ERROR: Hole detected outside polygon!";


We should change this to follow the style of the rest of our std::cerr statements.

That is:

std:cerr << "Error: Hole detected outside polygon! " << "ABC" << "XYZ"

Okay, I have added an issue for this.

src/inset_state/fill_with_density_rays.cpp

src/inset_state/inset_state.cpp

adisidev · 2024-11-28T03:08:15Z

src/inset_state/inset_state.cpp

 {
+  auto &geo_divs = original_area ? geo_divs_original_transformed_ : geo_divs_;


Maybe it would be better to just store this in a variable? Or do you mean the current area of the geo_divs_transformed, in which case probably we would want to change the name somehow.
Maybe "original_unit"?

Storing the area in a variable would require us to update that variable each time we update the geo_divs, which would require extra book keeping in many places, and this will increase the possibilities of bugs.

adisidev · 2024-11-28T03:10:50Z

src/main.cpp

-                << std::endl;
+      std::cerr
+        << "ERROR: Input GeoJSON is not a longitude-latitude map. Therefore, "
+           "it is not possible to produce an equal-area map."


Should we make this statement lighter. Or actually even remove it? Did you put it so that we can remember where this error came from?

I feel we should not remove it; this way we know that the --output_equal_area flag did not work for the given map. It is true that the statement contains redundant information because the first statement technically implies the second statement given the context. However, for a new user, it might not be obvious; and we do plan to display error messages to the users on the website.

adisidev · 2024-11-28T03:11:49Z

src/main.cpp

@@ -410,11 +412,18 @@ int main(const int argc, const char *argv[])

    // Rescale insets in correct proportion to each other
    inset_state.normalize_inset_area(
-      cart_info.cart_initial_total_target_area());
+      cart_info.cart_initial_total_target_area(),
+      false,


Maybe we should name the bools? @mgastner's opinion on style may be helpful here.

The alternative is we keep it like this and rely on IDE hints.

Good idea. Let's discuss it more.

src/main.cpp

adisidev · 2024-11-28T03:12:55Z

src/misc/ft_real_2d.cpp

@@ -15,7 +15,7 @@ void FTReal2d::set_array_size(const unsigned int i, const unsigned int j)
 void FTReal2d::allocate(const unsigned int lx, const unsigned int ly)
 {
  if (lx * ly <= 0) {
-    std::cerr << "Invalid array dimensions in FTReal2dArray::allocate_ft()"
+    std::cerr << "ERROR: Invalid array dimensions in FTReal2dArray::allocate_ft()"
              << std::endl;
    _Exit(98915);


On a side note, I wonder who is naming these exit codes.

adisidev · 2024-11-29T13:37:01Z

We should also resolve

/__w/cartogram-cpp/cartogram-cpp/src/cartogram_info/read_geojson.cpp:214:24: warning: comparison is always false due to limited range of data type [-Wtype-limits]
  214 |               return x < 0 || x > 127;
      |                      ~~^~~
[ 20%] Building CXX object CMakeFiles/cartogram.dir/src/cartogram_i

Here is the entire snippet. I am unsure whether it should be x should remain unsigned and the condition should just be x > 127, or we should make x signed. @nihalzp I don't think we should convert it to signed right away without first understanding how chars are read and processed.

        id_header.erase(
          std::remove_if(
            id_header.begin(),
            id_header.end(),
            [](unsigned char x) {
              return x < 0 || x > 127;
            }),
          id_header.end());
        id_header_ = id_header;

EDIT: I fixed this in f0536c9

Improve and automate `benchmark.sh`

…into inset-fix

nihalzp added 2 commits November 18, 2024 03:16

Shift original insets to position

245e901

Calculate bbox on transformed original insets

5b989f0

nihalzp added bug Something isn't working important This is higher priority than average ready-to-review priority: high labels Nov 17, 2024

nihalzp self-assigned this Nov 17, 2024

nihalzp added 2 commits November 18, 2024 03:54

Show ERROR output in a single line

a3e19b9

Calculate inset area on transformed original coordinates

e493c85

nihalzp requested a review from adisidev November 18, 2024 02:18

adisidev reviewed Nov 28, 2024

View reviewed changes

adisidev and others added 15 commits November 29, 2024 21:52

Use boost's own cmake.config (Fixes #223)

76495a5

Remove Redundant Negative for Check Unsigned Char

f0536c9

Use "none" as default if no csv is provided

d291175

Allow only geojson file for equal area map

d8d6872

Avoid potential division by zero when no csv is provided

08761ee

Add GeoDiv method to update id

63dabb7

Allow update of InsetState GeoDiv ids using id mapping

073ae2b

Use dump() when accesing nlohmann json string object

5e2d047

Only output the non simplified geosjon when equal area map is requested

47a2667

Do not store csv file name in CartogramInfo

0c5c00c

Parse GeoJSON before the CSV

b83a2b9

Refactor read_geojson to remove dependency of csv

3f0cd84

Refactor read_csv and handle the given inset position of GeoDivs

41634e8

Allows strip quotes when accessing nlohmann json string object

5ff5b2b

Merge branch 'main' into inset-fix

bb44ca0

nihalzp requested a review from adisidev December 2, 2024 08:01

adisidev added 2 commits December 3, 2024 19:57

Improve and automate benchmark.sh

89b589f

Merge pull request #224 from mgastner/improve-benchmark-sh

0a4a0ac

Improve and automate `benchmark.sh`

adisidev and others added 10 commits December 3, 2024 20:27

Add instructions to install for WSL users

6dc2b26

Add benchmarking instructions

3b1ad37

Move us_counties data to second column

ac950c8

Fix insets appearing too distant from each other

5a240b6

Revamp the logic of parser

41ce02e

Integrate new target area parser

c7dd6f3

Remove deprecated tests

3fa6e02

Use visual_variable_file_name

03507bf

Handle empty target area in CSV

527f4c7

Write function parameter names in header files

409fc33

nihalzp changed the title ~~Fix Issues with Shifting of Insets to Position~~ Fix Issues with Shifting of Insets to Position and String to Decimal Converter Dec 10, 2024

adisidev and others added 7 commits December 17, 2024 12:09

Add vector library

3046fd4

Calculate densities before quadtree construction

f7f9b5a

Merge remote-tracking branch 'origin/inset-fix' into inset-fix

07fc7e6

Add instructions to push changes to production

7dff687

Change CSV mismatch error to warning

ae2baf5

process_area_strs after checking validity

64aed88

Add functions to scale, move and transform points

ab67eff

adisidev force-pushed the inset-fix branch from d29e5f1 to ab67eff Compare December 18, 2024 09:10

adisidev added 8 commits December 18, 2024 17:11

Create function to write all insets as an svg

2009185

When creating CSV, base it on GeoJSON name

6bec777

Use transform_points function instead of long loop

e9de287

long_grid_side_length --> max_n_grid_rows_or_cols

c8920df

long_grid_side_length --> max_n_grid_rows_or_cols

118c861

Merge branch 'inset-fix' of https://github.com/mgastner/cartogram-cpp …

f44f74a

…into inset-fix

Write unblurred density after creating quadtree

0166544

plot quadtree corners on density image

28b23e4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Issues with Shifting of Insets to Position and String to Decimal Converter #218

Fix Issues with Shifting of Insets to Position and String to Decimal Converter #218

nihalzp commented Nov 17, 2024 •

edited

Loading

adisidev left a comment

adisidev Nov 28, 2024

nihalzp Dec 10, 2024

adisidev Nov 28, 2024

nihalzp Dec 10, 2024

adisidev Nov 28, 2024

nihalzp Dec 10, 2024

adisidev Nov 28, 2024

nihalzp Dec 10, 2024

adisidev Nov 28, 2024

adisidev commented Nov 29, 2024 •

edited

Loading

		{
		auto &geo_divs = original_area ? geo_divs_original_transformed_ : geo_divs_;

Fix Issues with Shifting of Insets to Position and String to Decimal Converter #218

Are you sure you want to change the base?

Fix Issues with Shifting of Insets to Position and String to Decimal Converter #218

Conversation

nihalzp commented Nov 17, 2024 • edited Loading

adisidev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisidev commented Nov 29, 2024 • edited Loading

nihalzp commented Nov 17, 2024 •

edited

Loading

adisidev commented Nov 29, 2024 •

edited

Loading