Skip to content

Conversation

@Tushar7012
Copy link
Contributor

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?

The movement package currently lacks native methods to save and load RegionOfInterest objects to disk. This is needed for:

What does this PR do?

Implements save/load functionality for ROI objects:

  1. to_file(path) method in BaseRegionOfInterest:

    • Saves ROI to JSON format with WKT geometry serialization
  2. from_file(path) classmethod in BaseRegionOfInterest:

    • Loads ROI from JSON file
    • Automatically determines correct subclass (LineOfInterest or PolygonOfInterest)
  3. _from_geometry() helper methods in LineOfInterest and PolygonOfInterest:

    • Reconstructs ROI objects from shapely geometry

References

Fixes #675

Related to #378 (broader ROI workflow)

How has this PR been tested?

Added new test file tests/test_unit/test_roi/test_save_load.py with 5 test cases:

  • test_save_and_load_polygon_roi - Round-trip test for PolygonOfInterest
  • test_save_and_load_line_roi - Round-trip test for LineOfInterest
  • test_save_and_load_polygon_with_hole - Test polygon with interior holes
  • test_load_nonexistent_file_raises - Error handling test
  • test_save_with_none_name - Test for unnamed ROIs

Is this a breaking change?

No, this adds new functionality without modifying existing behavior.

Does this PR require an update to the documentation?

API documentation will be auto-generated from the docstrings. The new methods include comprehensive docstrings with parameters, returns, and examples.

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

@Tushar7012 Tushar7012 force-pushed the feature/issue-675-roi-save-load branch from f3e4438 to 56c3eab Compare January 20, 2026 15:00
@sonarqubecloud
Copy link

@codecov
Copy link

codecov bot commented Jan 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (665412f) to head (56c3eab).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #735   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           34        34           
  Lines         2111      2138   +27     
=========================================
+ Hits          2111      2138   +27     

☔ 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.

@niksirbi
Copy link
Member

Thanks for taking the time to work on this, @Tushar7012. I’m going to close this PR for the reasons outlined below.

First, I had already assigned myself to work on #675, alongside #378 and #676. These three issues are closely related and are best implemented together to ensure a consistent design. In this case, I had indicated that I would be handling them by assigning the issues to myself. Please make sure to read the full issue context and check whether someone is already assigned (visible in the GitHub UI on the right-hand side) before starting work, as we want to avoid duplicated effort.

Regarding the implementation itself, there’s nothing inherently wrong with your approach. However, for movement, I think GeoJSON is a better fit than the WKT format you’ve used. In particular, we need to support saving not only single ROIs but also collections of regions, which GeoJSON handles naturally via its FeatureCollection construct. Like WKT, GeoJSON is also natively supported by shapely.

In fact, I was already in the process of implementing a GeoJSON-based solution for both individual ROIs and ROI collections when I came across this PR, and I’ll be opening a PR for that shortly. In the meantime, please refrain from working on #676 as well.

Thanks again for your interest and for engaging with the project.

@Tushar7012
Copy link
Contributor Author

Hi @niksirbi

Thank you for the detailed feedback and for taking the time to explain the rationale behind the GeoJSON approach — it makes complete sense given the need for ROI collections.

I apologize for the overlap; I'll make sure to check the Assignees section before starting work on future issues.

I appreciate the learning opportunity, and I look forward to contributing to other areas of the project!

@Tushar7012 Tushar7012 deleted the feature/issue-675-roi-save-load branch January 21, 2026 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement methods for saving/loading RegionOfInterest objects to file

2 participants