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

Coastlines s2s1 #461

Merged
merged 48 commits into from
Sep 7, 2023
Merged

Coastlines s2s1 #461

merged 48 commits into from
Sep 7, 2023

Conversation

lavender-liu
Copy link
Contributor

@lavender-liu lavender-liu commented Jun 27, 2023

Proposed changes

Create a new notebook coastal_erosion_Landsat_Sentinel.ipynb, which queries and selects best available products from Landsat, Sentinel-2 and Sentinel-1 for coastal erosion mapping. Merge DEA's upates on coastal related functions to DEAfrica and remove dependency on DEA.

Checklist (replace [ ] with [x] to check off)

  • Create a new notebook coastal_erosion_Landsat_Sentinel.ipynb, which queries and selects best available products from Landsat, Sentinel-2 and Sentinel-1 for coastal erosion mapping
  • Add its dependency functions into deafrica_tools.datahandling.py
  • Add/update functions in deafrica_tools.coastal script to keep updated with dea_tools
  • Replace dependencies on dea_tools for coastal erosion related scripts and notebooks
  • Update ceils on installing testing branches of deafrica-sandbox-notebooks and deafrica-coastlines once they are merged into main branches
  • Delete initially added notebooks coastal_erosion.ipynb, coastal_erosion_S2.ipynb and coastal_erosion_S1.ipynb

Closes issues (optional)

  • Closes Issue #000

@lavender-liu lavender-liu requested a review from fangfy June 27, 2023 13:08
@mickwelli
Copy link
Contributor

Sorry I'm late to the party here! Some thoughts from a discussion I had with @fangfy recently:

We have always run tandem dea_tools and deafrica_tools and there is considerable duplication e.g. in plotting functions. We don't have any DE Africa notebooks that point to dea_tools and I'd prefer to maintain that so that all our code is housed in a single repo. On the other hand, I also see the potential for duplicated/redundant time & effort which led us to thinking about whether there is scope for odc_tools that sits between programs, especially as new DE initiatives come online.

@mickwelli mickwelli marked this pull request as ready for review August 28, 2023 06:29
Copy link
Contributor

@mickwelli mickwelli left a comment

Choose a reason for hiding this comment

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

Looking good, a few plot & other minor updates. Over to @lisarebelo

Copy link
Contributor

@lisarebelo lisarebelo left a comment

Choose a reason for hiding this comment

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

Great notebook! Two minor comments - the figure "select_products.jpg" link is broken - I think it needs to be added to the supplementary data folder?

Second - wondering why the southern part of the islands was used as the test case, given the rocky shorelines. Is it due to data availability? If not, the norther section of the two island would make a better example given larger occurrence of sandy beaches.

@lavender-liu
Copy link
Contributor Author

Great notebook! Two minor comments - the figure "select_products.jpg" link is broken - I think it needs to be added to the supplementary data folder?

Second - wondering why the southern part of the islands was used as the test case, given the rocky shorelines. Is it due to data availability? If not, the norther section of the two island would make a better example given larger occurrence of sandy beaches.

Thank you both! @mickwelli @lisarebelo Not sure why the link to the figure "select_products.jpg" didn't work for you @lisarebelo but it works well for me (I asked Madeleine to double-check and it works for her as well) and I can see the figure in the supplementary data folder.
I've changed the default demonstration location to south of the island with sandy beaches as suggested.

@mickwelli
Copy link
Contributor

mickwelli commented Sep 6, 2023

@lavender-liu feel free to merge :) looks there are some conflicts to resolve first

@mickwelli mickwelli merged commit ceeddcf into main Sep 7, 2023
1 check passed
@mickwelli mickwelli deleted the coastlines_s2s1 branch September 7, 2023 02:50
Copy link
Contributor

@robbibt robbibt left a comment

Choose a reason for hiding this comment

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

This looks really fantastic @lavender-liu - great work, and very exciting to see this ready to be available via DE Africa!

I don't think it's a blocker to this work, but just wanted to chase up some of the older comments here:

We have always run tandem dea_tools and deafrica_tools and there is considerable duplication e.g. in plotting functions. We don't have any DE Africa notebooks that point to dea_tools and I'd prefer to maintain that so that all our code is housed in a single repo. On the other hand, I also see the potential for duplicated/redundant time & effort which led us to thinking about whether there is scope for odc_tools that sits between programs, especially as new DE initiatives come online.

Yeah, duplication between dea_tools and deafrica_tools has always been a challenge... for a lot of the content in here it's not really a big deal, as things like load_ard have to be customised for each repo anyway, and having a similar API in both repos makes a lot of sense.

For the coastal stuff though, it's a little trickier: these are tools we are actively publishing academically, so having a clear point of truth is really valuable for downstream users. For example, tools like subpixel_contours published here point to a specific branch of DEA Notebooks so we can make sure users always have access to the latest version of that function; I don't know if DE Africa's copy of that func has been kept up to date or is being regularly tested, which runs the risk that users might use the same function in two different locations and get different results.

By duplicating these tools across multiple repos we risk muddying the waters for our users, and dilating the scientific impact of our work (e.g. if a user decides to use pixel_tides for an African research project, what repo will they cite in their paper?). There's some short-term mitigations we could do to reduce this (e.g. making sure every DE Africa function clearly points to the source material in DEA Notebooks and links out to the relevant papers/citations), but I feel most of these issues would be more easily addressed by simply importing functions directly from their source in dea-tools, just as we currently do for any other open source repo.

Similarly, as DE Africa increasingly develops and publishes new tools, I feel it'd be best for DEA to reference and load that content directly from DE Africa Tools so that the work that's being done here can be recognised externally as well.

This is tricky stuff, but probably something we'll need to address sooner or later as both dea_tools and deafrica_tools continue to naturally mature and diverge. 🙂

@robbibt
Copy link
Contributor

robbibt commented Sep 7, 2023

(I guess this stuff is particularly relevant as we've recently made some major improvements to our tide modelling functions (model_tides and pixel_tides). If users naively assume DE Africa Tools has the latest version of those funcs, they'll be missing out on some pretty important improvements that ideally we'd like to get in the hands of all our users regardless of platform.
GeoscienceAustralia/dea-notebooks#1112)

@mickwelli
Copy link
Contributor

Thanks @robbibt, I agree this is an important consideration that, as you say, goes beyond just this PR. I've just odc slacked you with an update on some recent discussions about this issue. We can pick up discussions again in a Git issue or elsewhere if that would be helpful.

@lavender-liu
Copy link
Contributor Author

lavender-liu commented Sep 8, 2023

Thanks heaps @mickwelli for resolving the conflicts and merging the PR!

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.

None yet

6 participants