Skip to content

Conversation

@C8H10O2
Copy link

@C8H10O2 C8H10O2 commented Dec 2, 2025

Pull request type

  • Code changes (bugfix, features)
  • Code maintenance (refactoring, formatting, tests)

Checklist

  • Tests for the changes have been added (if needed)
  • Docs have been reviewed and added / updated
  • Lint (black rocketpy/ tests/) has passed locally
  • All tests (pytest tests -m slow --runslow) have passed locally
  • CHANGELOG.md has been updated (if relevant)

Current behavior

There is no built-in way to visualize these ellipses against a real-world map background directly within the Python environment (e.g., Jupyter Notebooks or saved logic). Users currently see only a white background with axes, which lacks context regarding the launch site, safety zones, or geographical landmarks.

The only current available option is to manually add a background image to the matplotlib figure.
See issue #890

New behavior

Users may specify the map background provider via the background parameter in MonteCarlo.plots.ellipses() and MonteCarlo.plots.ellipses_comparison().

Currently available options are:

  • "satellite" (uses Esri.WorldImagery)
  • "street" (uses OpenStreetMap.Mapnik)
  • "terrain" (uses Esri.WorldTopoMap)
  • or any contextily provider name from xyzservices.providers (e.g., "CartoDB.Positron").

Utilised on-demand imports, with proper handling of projection coordinate system conversions and network errors.

Breaking change

  • Yes
  • No

Copy link
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

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

Impressive work by @C8H10O2 !

Could you update monte carlo documentation and also the monte carlo example notebook to include this new feature?

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@C8H10O2 C8H10O2 force-pushed the enh/automatically-download-images branch from 0ee44b9 to f35dc18 Compare December 4, 2025 14:56
@C8H10O2
Copy link
Author

C8H10O2 commented Dec 4, 2025

Hi @Gui-FernandesBR , all review comments are addressed, and I've also updated the Monte Carlo documentation and example notebook to include the new feature.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 8 comments.

@Gui-FernandesBR
Copy link
Member

@C8H10O2 just a few more comments to solve and we are done. Almost there!

- Introduced a new method `_get_background_map` to fetch and display background maps using contextily.
- Added support for various map types: "satellite", "street", "terrain", and custom contextily providers.
- Updated `ellipses` and comparison methods to integrate background maps, enhancing visualization capabilities.
- Included error handling and warnings for missing dependencies and attributes.
- See issue RocketPy-Team#890
…rlo plots

- Added detailed docstring for the _get_background_map method, outlining parameters, return values, and background map options.
- Clarified usage of background types including "satellite", "street", "terrain", and contextily providers.
- Moved the import of imageio to be conditional upon the presence of an image, improving dependency management.
- This change ensures that imageio is only imported when necessary, optimizing performance and reducing unnecessary imports.
Copy link
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

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

LGTM

@Gui-FernandesBR
Copy link
Member

@C8H10O2 can you fix linters and tests please?

- Moved import statements for numpy and rocketpy.tools to the top of the test functions to adhere to best practices and improve readability.
- Added pylint disable comments for imports outside of the top-level to maintain code quality standards.
@C8H10O2
Copy link
Author

C8H10O2 commented Dec 5, 2025

@C8H10O2 can you fix linters and tests please?

I fixed the issues with the linters, but regarding the tests, it looks like it's because there is no rasterio wheel package available on Python 3.14. It needs to be compiled from source, and the compilation process depends on GDAL. I think the GitHub Actions configuration needs to be upgraded to automatically install the GDAL library.

@Monta120
Copy link

Monta120 commented Dec 5, 2025

@C8H10O2 Is there a way to get the same results with a default RocketPy dependency?I assume that (generally) we should avoid adding new libraries in the interest of optimization. (unless this a specific case @Gui-FernandesBR ? )

@C8H10O2
Copy link
Author

C8H10O2 commented Dec 5, 2025

@C8H10O2 Is there a way to get the same results with a default RocketPy dependency?I assume that (generally) we should avoid adding new libraries in the interest of optimization. (unless this a specific case @Gui-FernandesBR ? )

@Monta120 That is a valid concern regarding optimization, but I believe adding contextily is necessary in this specific case.

If we were to implement the map downloading feature manually, we would run into two main issues:

1. Maintenance burden: To allow users to simply pass background="satellite", we would have to hardcode and manually maintain a list of map provider APIs. If a provider changes their API and we don't update it immediately, the feature would break.

2. Redundancy: Building this functionality natively would essentially mean re-implementing contextily from scratch within RocketPy. It feels like "reinventing the wheel" rather than leveraging a library that already handles this efficiently.

@Gui-FernandesBR
Copy link
Member

@C8H10O2 Is there a way to get the same results with a default RocketPy dependency?I assume that (generally) we should avoid adding new libraries in the interest of optimization. (unless this a specific case @Gui-FernandesBR ? )

contextily is an optional requirement. No problem.

@Gui-FernandesBR
Copy link
Member

@C8H10O2 tests are still not passing on CI.

Is it passing locally?

@Monta120
Copy link

Monta120 commented Dec 5, 2025

I ran the tests locally. They passed (after installing contextily). The PR’s checker fails because the contextily library depends on rasterio, and rasterio needs the GDAL tools (gdal-config) to be installed on the CI machine; since GDAL is missing there, pip cannot build rasterio and aborts during pip install with the “A GDAL API version must be specified” error. What you can do is make the import completely optional in your helper inside monte_carlo_plots.py ( _get_background_map)(so remove the top import) then ask pytest to skip the tests on the machines that don't have contextily installed (using pytest.importorskip).
This change would make the feature optional and you can make the helper raise a clear error or warning that points users to install the optional dependency (such as " please run pip install contextily or "pip install "rocketpy[monte-carlo]"")
@Gui-FernandesBR Thoughts?

@Gui-FernandesBR
Copy link
Member

I ran the tests locally. They passed (after installing contextily). The PR’s checker fails because the contextily library depends on rasterio, and rasterio needs the GDAL tools (gdal-config) to be installed on the CI machine; since GDAL is missing there, pip cannot build rasterio and aborts during pip install with the “A GDAL API version must be specified” error. What you can do is make the import completely optional in your helper inside monte_carlo_plots.py ( _get_background_map)(so remove the top import) then ask pytest to skip the tests on the machines that don't have contextily installed (using pytest.importorskip). This change would make the feature optional and you can make the helper raise a clear error or warning that points users to install the optional dependency (such as " please run pip install contextily or "pip install "rocketpy[monte-carlo]"") @Gui-FernandesBR Thoughts?

Good option. Please proceed with the change but leaving a small comment explaining why the import was moved out of the top of the file

@C8H10O2
Copy link
Author

C8H10O2 commented Dec 5, 2025

What you can do is make the import completely optional in your helper inside monte_carlo_plots.py ( _get_background_map)(so remove the top import)

Hi,

The current implementation actually already handles this as you suggested: contextily is not imported at the top level.

Instead, I am using:

contextily = import_optional_dependency("contextily")

By utilizing the existing import_optional_dependency function in tools.py, contextily is loaded optionally. If the import fails, it raises an exception with guidance for the user:

raise ImportError(
            f"{package_name} is an optional dependency and is not installed.\n"
            + f"\t\tUse 'pip install {package_name}' to install it or "
            + "'pip install rocketpy[all]' to install all optional dependencies."
        ) from exc

@Gui-FernandesBR
Copy link
Member

I don't understand why the tests are failing on CI then. Sorry if I misunderstood (I work on several projects on GitHub, sometimes my reviews are a bit "too quick but shallow").

@Monta120 how hard would it to require the GDAL installation on CI? Would that mean that every user need to download it before using the feature?

@Monta120
Copy link

Monta120 commented Dec 5, 2025

What you can do is make the import completely optional in your helper inside monte_carlo_plots.py ( _get_background_map)(so remove the top import)

Hi,

The current implementation actually already handles this as you suggested: contextily is not imported at the top level.

Instead, I am using:

contextily = import_optional_dependency("contextily")

By utilizing the existing import_optional_dependency function in tools.py, contextily is loaded optionally. If the import fails, it raises an exception with guidance for the user:

raise ImportError(
            f"{package_name} is an optional dependency and is not installed.\n"
            + f"\t\tUse 'pip install {package_name}' to install it or "
        + "'pip install rocketpy[all]' to install all optional dependencies."
    ) from exc

What you can do is make the import completely optional in your helper inside monte_carlo_plots.py ( _get_background_map)(so remove the top import)

Hi,

The current implementation actually already handles this as you suggested: contextily is not imported at the top level.

Instead, I am using:

contextily = import_optional_dependency("contextily")

By utilizing the existing import_optional_dependency function in tools.py, contextily is loaded optionally. If the import fails, it raises an exception with guidance for the user:

raise ImportError(
            f"{package_name} is an optional dependency and is not installed.\n"
            + f"\t\tUse 'pip install {package_name}' to install it or "
            + "'pip install rocketpy[all]' to install all optional dependencies."
        ) from exc

You're right, I just looked at the code. Then I believe the only thing causing the issue is your contextily import inside test_ellipses_background_bounds2img_failure in tests/unit/simulation/test_monte_carlo_plots_background.py

Screenshot 2025-12-05 at 21 13 11

Could you remove that import and then replace it with a module level pytest.importorskip? As long as contextily remains an optional dependancy the checker shouldn't fail

- Added pytest.importorskip for contextily in both integration and unit test files to ensure tests are only run if the required library is installed.
- Replaced direct import of contextily with pytest.importorskip to ensure tests are skipped if the library is not available, enhancing test robustness.
@Monta120
Copy link

Monta120 commented Dec 5, 2025

how hard would it to require the GDAL installation on CI? Would that mean that every user need to download it before using the feature?

I don't understand why the tests are failing on CI then. Sorry if I misunderstood (I work on several projects on GitHub, sometimes my reviews are a bit "too quick but shallow").

@Monta120 how hard would it to require the GDAL installation on CI? Would that mean that every user need to download it before using the feature?

If it's only added to the CI and not as a dependency then no, users wouldn't need to install it nor would it install automatically when a user clones the rocketpy repository. I also don't believe we as contributors can modify the CI outside of our fork.
That might not be necessary though, I don't see why an importor skip wouldn't work. Let's see if the checkers pass after the latest commit.

@Monta120
Copy link

Monta120 commented Dec 5, 2025

@C8H10O2 I think a module level pytest.importorskip would be better (at the top of the file)

@C8H10O2
Copy link
Author

C8H10O2 commented Dec 5, 2025

I don't understand why the tests are failing on CI then. Sorry if I misunderstood (I work on several projects on GitHub, sometimes my reviews are a bit "too quick but shallow").

@Monta120 how hard would it to require the GDAL installation on CI? Would that mean that every user need to download it before using the feature?

1. The CI Failure: Simply put, rasterio (a dependency of contextily) does not yet have pre-built wheels for Python 3.14. Consequently, pip tries to build it from source. This compilation requires the GDAL system library, which isn't present in the default GitHub Actions environment, causing the installation to fail.

2. Fixing CI: It shouldn't be too complicated. I will try updating .github/workflows/test_pytest.yaml shortly to automatically install GDAL in the CI environment.

3. User Impact: For now, yes, users on Python 3.14 would need GDAL installed system-wide. However, this is temporary. According to the rasterio issue #3419, Python 3.14 wheels should be released soon. Once available, users won't need to build from source or install GDAL manually.

@C8H10O2
Copy link
Author

C8H10O2 commented Dec 5, 2025

@C8H10O2 I think a module level pytest.importorskip would be better (at the top of the file)

Please check the previous commit, I've already implemented module-level pytest.importorskip in both the unit test and integration test files.

@Monta120
Copy link

Monta120 commented Dec 5, 2025

I don't understand why the tests are failing on CI then. Sorry if I misunderstood (I work on several projects on GitHub, sometimes my reviews are a bit "too quick but shallow").
@Monta120 how hard would it to require the GDAL installation on CI? Would that mean that every user need to download it before using the feature?

1. The CI Failure: Simply put, rasterio (a dependency of contextily) does not yet have pre-built wheels for Python 3.14. Consequently, pip tries to build it from source. This compilation requires the GDAL system library, which isn't present in the default GitHub Actions environment, causing the installation to fail.

2. Fixing CI: It shouldn't be too complicated. I will try updating .github/workflows/test_pytest.yaml shortly to automatically install GDAL in the CI environment.

3. User Impact: For now, yes, users on Python 3.14 would need GDAL installed system-wide. However, this is temporary. According to the rasterio issue #3419, Python 3.14 wheels should be released soon. Once available, users won't need to build from source or install GDAL manually.

As I said, I'm not sure you have the required permissions to edit the CI beyond our fork.And if this check passes then there's no need to touch the CI at all.

@Gui-FernandesBR
Copy link
Member

I will take a look at it during the weekend

1 similar comment
@Gui-FernandesBR
Copy link
Member

I will take a look at it during the weekend

@C8H10O2
Copy link
Author

C8H10O2 commented Dec 6, 2025

@Gui-FernandesBR
Regarding the CI and user experience, I spent a few hours investigating the feasibility of building rasterio from source today.

While installing GDAL and compiling rasterio on Linux and macOS is straightforward (using system package managers), doing so on Windows is virtually impossible. It requires a very complex environment configuration that is definitely not suitable for a standard user guide. Since Python 3.14 is so new, pre-built binaries are also not available on conda-forge yet.

Given this, I propose we temporarily restrict the dependency in pyproject.toml until rasterio releases wheels for 3.14. We can modify the optional dependencies like this:

monte-carlo = [
    ...
    "contextily>=1.0.0; python_version < '3.14'",
]

This ensures contextily is only installed on supported Python versions. I will also update the documentation to reflect this limitation.

I have already tested this configuration on my fork, and the CI pipeline passed successfully.

If you agree with this solution, please let me know, and I will submit a commit to update pyproject.toml and the documentation.

@Gui-FernandesBR
Copy link
Member

@C8H10O2 brilliant!! Please move forward with your implementation.

If you don't mind... Something we usually do in open-source project when facing problems like that is to raise an issue on the dependency repo so we can follow the development of GDAL binaries for py3.14+windows from the contextly side.
However, this is a "plus" or a "nice to have". I won't block the PR if you don't proceed with that.

…nal.txt

- Modified contextily dependency in pyproject.toml to conditionally require it for Python versions below 3.14.
@C8H10O2
Copy link
Author

C8H10O2 commented Dec 7, 2025

@Gui-FernandesBR
Thanks! I've pushed the changes to pyproject.toml and the docs.

Regarding the upstream issue: As I mentioned in my previous comment, there is already an active issue on the rasterio repo tracking this.

According to the rasterio issue #3419, Python 3.14 wheels should be released soon.

Since I linked it here, GitHub has automatically cross-referenced this PR in their thread, so the maintainers can already see that downstream projects (like us) are waiting on this.

Given that there is no clear timeline on their side yet, I agree that merging this temporary fix is the best way forward. I have subscribed to that issue, and I'll be happy to submit a follow-up PR to revert this restriction once the wheels are officially released.

@codecov
Copy link

codecov bot commented Dec 8, 2025

Codecov Report

❌ Patch coverage is 92.00000% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.13%. Comparing base (9cf3dd4) to head (e450d25).
⚠️ Report is 23 commits behind head on develop.

Files with missing lines Patch % Lines
rocketpy/plots/monte_carlo_plots.py 91.30% 6 Missing ⚠️
rocketpy/tools.py 93.54% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #896      +/-   ##
===========================================
+ Coverage    80.27%   81.13%   +0.86%     
===========================================
  Files          104      107       +3     
  Lines        12769    13742     +973     
===========================================
+ Hits         10250    11150     +900     
- Misses        2519     2592      +73     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

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

LGTM,

@Gui-FernandesBR Gui-FernandesBR merged commit a25fd25 into RocketPy-Team:develop Dec 8, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Backlog

Development

Successfully merging this pull request may close these issues.

ENH: Automatically downloads satellite images from the internet

3 participants