Skip to content
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.

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

fix(Indian Map Changes): fixed-Indian-map-border #24927

Merged
merged 3 commits into from
Sep 30, 2023

Conversation

Yaswanth-Perumalla
Copy link
Contributor

@Yaswanth-Perumalla Yaswanth-Perumalla commented Aug 9, 2023

SUMMARY

The previous Indian border was inaccurate, and I've now made the required adjustments to the existing border of India.

BEFORE

image

AFTER

image

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Bug references:

Closes #20452
Closes #19729

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

@Yaswanth-Perumalla Yaswanth-Perumalla changed the title fixed-indian-map-border fix(Indian Map FIx): fixed-Indian-map-border Aug 9, 2023
@Yaswanth-Perumalla Yaswanth-Perumalla changed the title fix(Indian Map FIx): fixed-Indian-map-border fix(Indian Map Fix): fixed-Indian-map-border Aug 9, 2023
@Yaswanth-Perumalla Yaswanth-Perumalla changed the title fix(Indian Map Fix): fixed-Indian-map-border fix(Indian Map Changes): fixed-Indian-map-border Aug 9, 2023
@john-bodley
Copy link
Member

john-bodley commented Aug 9, 2023

Thanks @Yaswanth-Perumalla for the PR. Would you mind including before and after screenshots so reviewers can visualize the difference?

Also any additional context you can provide in terms of what was wrong and/or why your proposed solution is correct would be helpful.

@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Merging #24927 (55a7ace) into master (ec9e9a4) will not change coverage.
Report is 3 commits behind head on master.
The diff coverage is n/a.

❗ Current head 55a7ace differs from pull request most recent head 2706530. Consider uploading reports for the commit 2706530 to get more accurate results

@@           Coverage Diff           @@
##           master   #24927   +/-   ##
=======================================
  Coverage   69.00%   69.00%           
=======================================
  Files        1906     1906           
  Lines       74133    74133           
  Branches     8208     8208           
=======================================
  Hits        51153    51153           
  Misses      20857    20857           
  Partials     2123     2123           
Flag Coverage Δ
javascript 55.84% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@john-bodley
Copy link
Member

@Yaswanth-Perumalla it seems that your updated map may include disputed territories. I think @villebro might have worked on a similar issue in the past and likely should weigh in here. In the case of a dispute I believe we allow all disputing territories include the region in their map.

@villebro
Copy link
Member

@Yaswanth-Perumalla it seems that your updated map may include disputed territories. I think @villebro might have worked on a similar issue in the past and likely should weigh in here. In the case of a dispute I believe we allow all disputing territories include the region in their map.

We take the stance that disputed areas can be featured on multiple maps, as long as there is an ISO code to back it up. @Yaswanth-Perumalla would you mind adding a reference to where you obtained the updated polygon data for this PR? Also, if you used the Notebook in this repo, it would be nice if you could also update that as part of this PR so that it's in sync with the current state of the rendered map regions.

@rusackas
Copy link
Member

rusackas commented Aug 28, 2023

@Yaswanth-Perumalla there are a handful of people on Slack very excited to merge this PR. Can you give us a response on @villebro's question above?

@Yaswanth-Perumalla
Copy link
Contributor Author

@villebro @rusackas I have not utilized any notebooks in this repository. Can you guide me on where I should make updates?

@villebro
Copy link
Member

villebro commented Sep 1, 2023

@Yaswanth-Perumalla this is the notebook that's been used to generate most (or all) maps: https://github.com/apache/superset/tree/master/superset-frontend/plugins/legacy-plugin-chart-country-map/scripts Can you check if that generates the correct polygons, and if not, maybe add a note to it so someone doesn't overwrite these in the future.

@Yaswanth-Perumalla
Copy link
Contributor Author

@villebro I have updated the notes. Kindly check and let me know if you need any further changes.

@Yaswanth-Perumalla
Copy link
Contributor Author

@rusackas I've encountered a failed check after making updates to the notes. Can you assist me in understanding the problem?

@villebro
Copy link
Member

If you check the failed CI workflow, it appears to complain about a missing newline at the end of the file. If you need help resolving it I can help, but feel free to take a stab at it first.

1 similar comment
@villebro
Copy link
Member

If you check the failed CI workflow, it appears to complain about a missing newline at the end of the file. If you need help resolving it I can help, but feel free to take a stab at it first.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your patience

@villebro villebro merged commit 0d0a81c into apache:master Sep 30, 2023
@michael-s-molina michael-s-molina added v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch and removed v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch labels Oct 2, 2023
@rusackas rusackas added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Nov 21, 2023
@michael-s-molina michael-s-molina removed the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Dec 4, 2023
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024
@amanmaurya
Copy link

The map of India is incorrect after upgrading to version 4.0.1

@rusackas
Copy link
Member

The map of India is incorrect after upgrading to version 4.0.1

Eek! If you want to open a PR, feel free, but we'll try to assess/fix ASAP. I even wrote a blog post about how to make such a contribution, and how we're trying to be better about keeping these things current and permanent.

@PushpenderSaini0
Copy link
Contributor

The map of India is incorrect after upgrading to version 4.0.1

Eek! If you want to open a PR, feel free, but we'll try to assess/fix ASAP. I even wrote a blog post about how to make such a contribution, and how we're trying to be better about keeping these things current and permanent.

Hi i have raised a PR that restores changes that were made in this PR using the new GeoJson Generator script !

cc @amanmaurya

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XXL 🚢 3.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect Map of India Update geojson for India's map
9 participants