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

repo structure #16

Closed
yunjunz opened this issue Apr 21, 2022 · 8 comments · Fixed by #21
Closed

repo structure #16

yunjunz opened this issue Apr 21, 2022 · 8 comments · Fixed by #21

Comments

@yunjunz
Copy link
Contributor

yunjunz commented Apr 21, 2022

Seeing the growing COMPASS repo, I am thinking of the following changes:

  1. rename configuration files from cslc_s1.yaml to s1_cslc.yaml, to be consistent with the script name s1_cslc.py
  2. move schemas and defaults from within utils to the same level as utils for shoter path, since users will check them frequently
  3. create a bin directory for executables, like s1_cslc.py, as they are really executable scripts. Update: we could add entry_points for executables, without moving them to a bin folder.

These lead to a structure as below:

/COMPASS
    /bin
        s1_cslc.py
        s1_gslc.py
    /src/compass
        s1_rdr2geo.py
        s1_geo2rdr.py
        s1_resample.py (renamed from s1_resample_burst.py)
        ...
        /utils
            helpers.py
            geo_grid.py (renamed from geogrid.py)
            radar_grid.py (renamed from reference_radar_grid.py)
            runconfig.py
            ...
        /defaults
            s1_cslc.yaml (renamed from cslc_s1.py)
            s1_gslc.yaml (renamed from geo_cslc_s1.yaml)
        /schemas
            s1_cslc.yaml
            s1_gslc.yaml
    /tests
        ...

Let me know what do you think please @LiangJYu @vbrancat @hfattahi .

@hfattahi
Copy link
Contributor

I like the idea. For OPERA we chose the term CSLC to represent Coregistered SLC in general regardless if they are in radar or geo-coordinates. If we want to avoid confusion in that regard, then I suggest to use:

s1_cslc_radar.py
s1_cslc_geo.py

That being said, the name that I really like for GSLC is geocode_slc.py

@yunjunz
Copy link
Contributor Author

yunjunz commented Apr 21, 2022

That being said, the name that I really like for GSLC is geocode_slc.py

I assume you meant s1_geocode_slc.py? I like it a lot too.

If the geocode SLC executable script can be as short as the current s1_cslc.py, we can merge it into s1_cslc.py and add an option in the yaml file to choose between geo or radar grid. That would be even simpler. And we could keep one yaml file also, instead of two.

@LiangJYu
Copy link
Contributor

LiangJYu commented Apr 21, 2022

I like the changes. A few questions/comments:

How does bin get installed? I have no idea and an explanation would be very useful. I've been installing COMPASS into my conda env and running with python -m and would like to learn more.

Regarding the common executable, were you thinking the following?

/COMPASS
    /bin
        s1_cslc.py (runconfig input determines geo or radar coordinate processing path)
    /src/compass
        s1_rdr2geo.py
        s1_geo2rdr.py
        s1_resample.py (renamed from s1_resample_burst.py)
        s1_geocode_slc.py (renamed from geo_cslc.py)
        ...
        /utils
            helpers.py
            geo_grid.py (renamed from geogrid.py)
            radar_grid.py (renamed from reference_radar_grid.py)
            runconfig.py (combination of both runconfig.py's)
            ...
        /defaults
            s1_cslc.yaml (renamed from cslc_s1.yaml and merged from 2 current defaults)
        /schemas
            s1_cslc.yaml (merge of 2 current schemas)
    /tests
        ...

I like the cleanliness and the additional logic doesn't seem too troublesome. That said, I also like the descriptive clarity of current verbose structure.

@hfattahi
Copy link
Contributor

I think the run config for radar and geo slc have fundamental differences which we only introduce un-necessary work and complication to the workflow for gaining little or nothing. I suggest we keep the cslc workflows separate for geo and radar coordinates.

@yunjunz
Copy link
Contributor Author

yunjunz commented Apr 21, 2022

We could make scripts available on the command line using entry_points in setup.cfg. With this, pip install will create a pointer file in the python environment's bin folder, e.g. $CONDA_PREFIX/bin. Here is an example in mintpy, Joseph has a detailed explanation here.

Within compass itself, the bin folder is actually not needed, so that makes things simpler.

@vbrancat
Copy link
Contributor

I like the changes and the suggestions given above. My preference is that the repo re-organization is done after the geocoded burst PR gets merged (less work in addressing conflicts)

@vbrancat
Copy link
Contributor

@yunjunz I would like to close this issue. Will you take care of this? Now is a good time to re-organize the repo before we submit other PRs.

@yunjunz
Copy link
Contributor Author

yunjunz commented May 11, 2022

Yes, I will issue a PR soon.

Update: below is what I shooting for, after accommodating our discussions above and offline.

/COMPASS
    /src/compass
        s1_cslc.py (add an option to switch bwt geo/radar and call s1_geocode_slc.py for geo workflow)
        s1_rdr2geo.py
        s1_geo2rdr.py
        s1_geocode_slc.py (renamed from geo_cslc.py)
        s1_resample.py (renamed from s1_resample_burst.py)
        ...
        /utils
            helpers.py
            geo_grid.py (renamed from geogrid.py)
            geo_metadata.py (renamed from geo_cslc_metadata.py)
            radar_grid.py (renamed from reference_radar_grid.py)
            runconfig.py
            del unwrap_namespace.py (duplicate to wrap_namespace.py)
            ...
        /defaults
            s1_cslc_radar.yaml (renamed from cslc_s1.py)
            s1_cslc_geo.yaml (renamed from geo_cslc_s1.yaml)
        /schemas
            s1_cslc_radar.yaml
            s1_cslc_geo.yaml
    /tests
        ...

@yunjunz yunjunz linked a pull request May 12, 2022 that will close this issue
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 a pull request may close this issue.

4 participants