isce-framework / spurt

Spatial and Temporal phase Unwrapping for InSAR time-series
https://spurt.readthedocs.io/
Apache License 2.0
20 stars 4 forks source link

Fully working merge pipeline #14

Closed piyushrpt closed 3 months ago

piyushrpt commented 4 months ago
scottstanie commented 4 months ago

(I realize that you're actively pushing, so if you already noticed, please disregard) I saw the second to last commit has from Pathlib import Path, which is failing from the capital C.

Have you run into intermittent test failures for the unwrap_one test?

If I do

$ pytest --randomly-seed=2713237683 test/mcf/test_ortools.py::test_unwrap_one

I get

FAILED test/mcf/test_ortools.py::test_unwrap_one - AssertionError: assert 2.6645352591003757e-15 > 1

but other seeds work okay for me.

piyushrpt commented 4 months ago

😢 - good catch. I will include it as part of the next couple of commits later today.

scottstanie commented 4 months ago

Have you come across this ORTools error?

$ python -m spurt.workflows.emcf -i . -w 3
2024-05-21 17:45:16,337 [49229] [INFO] spurt: Using Hop3 Graph in time with 21 epochs.
2024-05-21 17:45:16,338 [49229] [INFO] spurt: Using existing tiles file: emcf_tmp/tiles.json
2024-05-21 17:45:16,338 [49229] [INFO] spurt: Processing tile: 1
2024-05-21 17:45:28,906 [49229] [INFO] spurt: Time steps: 57
2024-05-21 17:45:28,906 [49229] [INFO] spurt: Number of points: 421201
2024-05-21 17:45:28,906 [49229] [INFO] spurt: Temporal: Number of interferograms: 57
2024-05-21 17:45:28,906 [49229] [INFO] spurt: Temporal: Number of links: 1262032
2024-05-21 17:45:28,906 [49229] [INFO] spurt: Temporal: Number of cycles: 37
2024-05-21 17:45:28,906 [49229] [INFO] spurt: Temporal: Preparing batch 1/26
2024-05-21 17:45:29,033 [49229] [INFO] spurt: Temporal: Unwrapping batch 1/26
Processing batch of 50000 with 3 threads
multiprocessing.pool.RemoteTraceback:
"""
Traceback (most recent call last):
  File "/Users/staniewi/miniconda3/envs/mapping-311/lib/python3.11/multiprocessing/pool.py", line 125, in worker
    result = (True, func(*args, **kwds))
                    ^^^^^^^^^^^^^^^^^^^
  File "/Users/staniewi/repos/spurt/src/spurt/mcf/_ortools.py", line 360, in wrap_solve_mcf
    return (ind, solve_mcf(es, ed, rr, cc, rc))
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/staniewi/repos/spurt/src/spurt/mcf/_ortools.py", line 323, in solve_mcf
    raise ValueError(errmsg)
ValueError: MCF unbalanced. Sum of residues in non-zero.
"""

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/Users/staniewi/repos/spurt/src/spurt/workflows/emcf/__main__.py", line 5, in <module>
    sys.exit(main())
             ^^^^^^
  File "/Users/staniewi/repos/spurt/src/spurt/workflows/emcf/_cli.py", line 96, in main
    unwrap_tiles(stack, g_time, gen_settings, slv_settings)
  File "/Users/staniewi/repos/spurt/src/spurt/workflows/emcf/_unwrap.py", line 54, in unwrap_tiles
    uw_data = solver.unwrap_cube(wrap_data)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/staniewi/repos/spurt/src/spurt/workflows/emcf/_solver.py", line 132, in unwrap_cube
    grad_space: np.ndarray = self.unwrap_gradients_in_time(
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/staniewi/repos/spurt/src/spurt/workflows/emcf/_solver.py", line 223, in unwrap_gradients_in_time
    flows = self._solver_time.residues_to_flows_many(
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/staniewi/repos/spurt/src/spurt/mcf/_ortools.py", line 277, in residues_to_flows_many
    for res in mp_tasks:
  File "/Users/staniewi/miniconda3/envs/mapping-311/lib/python3.11/multiprocessing/pool.py", line 873, in next
    raise value
ValueError: MCF unbalanced. Sum of residues in non-zero.
piyushrpt commented 4 months ago

Hmm .. I haven't run into that unbalanced error before. This should ensure that sum is zero - https://github.com/piyushrpt/spurt/blob/f75d2b39b3680b242e2298b11df7367d0976fdd1/src/spurt/workflows/emcf/_solver.py#L219

I will add a cast to integer here just like in mcf solver.

piyushrpt commented 4 months ago

The other possibility is that there are nans in the data. In which case, the sum of residues will end up being a nan and that breaks the balanced check

scottstanie commented 4 months ago

You were correct, there were a few nan pixels in a couple images.

Perhaps we could build in some nan check for a clearer error, or a conversion like nan_to_num if there's no harm in converting to zero.

@gmgunter thoughts on those options?

piyushrpt commented 4 months ago

There are only a couple of places where pixel selection takes place. Example: https://github.com/piyushrpt/spurt/blob/911f0d4021dd7ee0488017ccddcf56ced997414e/src/spurt/workflows/emcf/_tiling.py#L21

I would suggest not needing nan checks on phase data - but if temporal coherence can be set to nan / zero for such pixels - the workflow can already handle it.

scottstanie commented 4 months ago

Just noting that my tests are running through, while there's small possible changes/additions, the biggest thing that will help calling this from dolphin will be to split the _cli.main into one function that gets args from sys.argv (something like _get_cli_args(args: list[str] = None), and another that has all the inputs listed out that's more callable in the middle of a workflow.

Since that might take a little code juggling, we can wait until this is merged to split that up

piyushrpt commented 3 months ago

This has been rebased and ready for review. I will wait for this to be merged before starting the next set.

scottstanie commented 3 months ago

in workflows/emcf/init.py:

from ._settings import GeneralSettings, MergerSettings, SolverSettings, TilerSettings
from ._solver import EMCFSolver as Solver

__all__ = [
    "Solver",
    "MergerSettings",
    "GeneralSettings",
    "SolverSettings",
    "TilerSettings",
]

GeneralSettings currently misspelled in the init, and having the other options exposed seems useful