-
Notifications
You must be signed in to change notification settings - Fork 128
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
Draft of Near Real-time Burnt Area Mapping DEA notebook for review #1018
Conversation
Draft notebook for mapping near real-time fires.
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
View / edit / reply to this conversation on ReviewNB geoscience-aman commented on 2023-04-04T05:54:54Z I'm being pedantic here but perhaps we rename the filename to be in sentence case to be consistent with other notebooks in Matt-dea commented on 2023-04-18T06:23:21Z Yep I agree :) Updated Matt-dea commented on 2023-04-18T07:09:13Z Although i noticed that some of the other examples are a bit mixed haha, I will keep DEA capitalised |
View / edit / reply to this conversation on ReviewNB geoscience-aman commented on 2023-04-04T05:54:55Z As discussed, I'll let Erin review this text in detail but here are a few comments:
Matt-dea commented on 2023-04-19T00:20:16Z
|
View / edit / reply to this conversation on ReviewNB geoscience-aman commented on 2023-04-04T05:54:56Z Not sure why each dot point does not show on its own line. Also, can link these headings to the corresponding sections in the notebook. Matt-dea commented on 2023-04-19T00:21:23Z Thats weird, the dot points appear on their own line for me? I will have to look into this further! EDIT: Resolved Matt-dea commented on 2023-04-19T00:22:30Z As for the links, I agree that is a good idea but see that it is not done in the other notebooks. I will not implement just to make things consistent :) |
View / edit / reply to this conversation on ReviewNB geoscience-aman commented on 2023-04-04T05:54:57Z As discussed, perhaps it is worth adding a word of caution here to say that selecting too big of a ROI might run out of memory depending on where you are running it? geoscience-aman commented on 2023-04-18T00:19:16Z To clarify, when I said "where you are running it", I mean how much memory your sandbox environment has.
Matt-dea commented on 2023-04-19T01:52:42Z Great point, I have added a disclaimer for large AOI's with a suggestion to reduce the AOI or increase the min_gooddata values if a crash occurs. |
Hey @Matt-dea and @erin-telfer - will get on this first thing next week, sorry for the delay! |
To clarify, when I said "where you are running it", I mean how much memory your sandbox environment has.
View entire conversation on ReviewNB |
Yep I agree :) View entire conversation on ReviewNB |
Although i noticed that some of the other examples are a bit mixed haha, I will keep DEA capitalised View entire conversation on ReviewNB |
View entire conversation on ReviewNB |
Thats weird, the dot points appear on their own line for me? I will have to look into this further! View entire conversation on ReviewNB |
As for the links, I agree that is a good idea but see that it is not done in the other notebooks. I will not implement just to make things consistent :) View entire conversation on ReviewNB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Matt-dea, this is awesome work! The notebook is great: nicely documented and clean code, and the outputs are fantastic! I've put a bunch of feedback below - it's all minor stuff, and the notebook is already very close to being ready to publish!
-
This is a tricky one since the notebook is primarily focused on NRT analysis... but the biggest issue I had was that the use of
datetime.today().strftime("%Y-%m-%d")
to setnrt_date_end
meant that when I re-ran it today on April 19 , the notebook produced no fire polygon results because there wasn't a fire during the analysis period. I think this will be confusing to users as they'll end up with an empty result and wonder if it was something they did wrong... it will probably be better to hard-codenrt_date_end
to a specific date that we know includes a nice fire, e.g.nrt_date_end = "2023-03-31"
, but then provide really clear instructions to set this tonrt_date_end = datetime.today().strftime("%Y-%m-%d")
for today's date. -
I'd rename
Near_Realtime_Burnt_Area_Mapping.ipynb
toBurnt_area_mapping_near_real_time.ipynb
. That way it will appear side-by-side with the existing notebook and be easier to find. -
For the "safety of life decisions" warning, we could consider using an actual alert box like this - I think we could make more of these kind of boxes across DEA Notebooks, they're great for catching attention:
<div class="alert alert-block alert-danger"> <b>IMPORTANT</b>: This notebook and the datasets visualised and produced are not to be used for safety of life decisions. For local updates and alerts, please refer to your state emergency or fire service. </div>
-
Change "Sentinel 2a" and "Sentinel 2b" in the intro to "Sentinel-2A" and "Sentinel-2B"
-
Rather than using bold in the "Description" contents list, another option could be to hyperlink them instead as we've recently started doing in recent notebooks (although we need to update the template notebook) - each heading has a "#Blah" format anchor link that you can copy from the URL bar if you click on them: e.g.:
1. [Getting Started and Defining an AOI](#1.-Getting-started-and-defining-an-AOI)
-
This section says the default is -1 but the code says 1:
# Index selection for the Near Real-time image chosen above. The default value of -1 selects the most recent image regardless of suitability.
nrt_img_index = 1
- A completely option one: a lot of the notebook is taken up with plotting code - potentially some of this could be simplified by creating a reproducible function that handles the plotting (it could live in
dea_tools.plotting
), and then re-use that throughout the notebook. The tricky thing would be some of the cells that combine both Landsat and S2 data, but you could probably just run that function seperately on both sources of data rather than having them on the same plot. 😃
Ah, two more related things:
|
Great point, I have added a disclaimer for large AOI's with a suggestion to reduce the AOI or increase the min_gooddata values if a crash occurs. View entire conversation on ReviewNB |
* Readme update (#1019) * test this * permalink to the template notebook * trying to get the link working * Update README.rst * Update README.rst --------- Co-authored-by: Nathan <[email protected]> * upgrade docker-compose postgres image version to 13 * Update bandindices.py (#1022) Added Normalised Difference Turbidity Index to the 'calculate_indices' tool. NB. acronym 'NDTI' is already in use for Tillage Index. Acronym 'TI' used in lieu. * Readme Update (#1025) Updating to include link back to wiki for approving/reviewing pull requests. * Tiny doc update to remote build trigger (#1026) * Update trigger_docs.yaml Add explanation of triggering build in other doc repo * Formatting suggestions from review * Fix `pixel_tides` issues when `ds` has no time dimension (#1028) * Update pixel_tides func to cover edge cases * Sync with develop branch * Hard code dimension order instead of getting dims from ds * Clean code * Update USAGE.rst (#1029) I've added a recently accepted manuscript where I used DEA to calulcate Geomedians and show landscape changes. And the relevant years where we've used DEA sandbox in our remote sensing topics at Flidners university. Additionally added a conference presentation from AEO22 from Earth Observation Australia 2022 in Brisbane, * Update spellcheck_wordlist.txt * Update DEA Notebooks directory structure (#1027) * New interactive apps folder, moved appropriate notebooks inside * Deleted scientific_workflows * rename DEA_datasets folder to DEA_products * updated 17 notebook references from DEA_datasets to DEA_products * Rename frequently used code * Fix incorrect repo folders * Revert to before dir rename * Rename frequently used code * Update readme for DEA_products and How_to_guides * replace notebook references to Frequently_used_code with How_to_guides * replace notebook references to frequently used code with How_to_guides * replaced notebook references to DEA_datasets * Add readme to interactive apps * updated Real_world_examples readme * Remove defunct tests (replaced by nbval --lax) * Update readme to include interactive apps * Use stable branch * Update real world examples format * Update guided tutorial link * Update template to remove mention of scientific workflows and Scripts dir * Fix formatting * Fix formatting * Last attempt to fix template notebook formatting * Final final fix of formatting --------- Co-authored-by: erialC-P <[email protected]> * Fix geocoding func to still work if geocoding service is down (#1030) * Fix geocoding func to still work if geocoding service is down * Fix format * Extract coastline app from product notebook and move to Interctive_apps (#1032) * Update spellcheck_wordlist.txt (#1037) add: Ai Fanson Fickas Ronan * Update USAGE.rst (#1036) Added Wetlands paper!! * Replace conference paper with Wetlands paper (#1034) updated citation list with paper * Update plotting.py Added a tool named 'plot_variable_images' primarily for use in the "Burnt_area_mapping_near_real_time" notebook. * Update plotting.py * Update plotting.py * small correction to new tool in plotting.py Added extra check to handle error when the input xarray dataset has 0 images. * Update plotting.py Actioned suggestion to raise error. * Add Sentinel-2 and Landsat option to animation notebook (#1043) * Add Sentinel-2 and Landsat option to animation notebook * Set default resolutions * Fix bug where choosing combined product didn't trigger data reload * Update functions to use `odc-geo` and add additional content to tests (#1039) * Make geobox adding func more generic * Remove deprecated map_shapefiles func and replace with .explore * Use odc-geo geobox in xr_animation * Add geomedian collection option to calculate_indices * Update animation notebook to use geomedian so we can add it to tests * Update tests to run on animation notebook * Fix collection number in segmentation notebook * Update rgb func to use odc-geo * Fix attributes being dropped by xr.where, update deprecation version * Remove assign_crs from rgb (not needed), and add error handling to xr_animation * Use better error * Re-run affected notebooks * Update collection param in ML notebooks * Add manual reload trigger to layer selection (#1044) --------- Co-authored-by: Bex Dunn <[email protected]> Co-authored-by: Nathan <[email protected]> Co-authored-by: Pin Jin <[email protected]> Co-authored-by: Pin Jin <[email protected]> Co-authored-by: Matt-dea <[email protected]> Co-authored-by: LaurenSchenk1 <[email protected]> Co-authored-by: Robbi Bishop-Taylor <[email protected]> Co-authored-by: mdasi00 <[email protected]> Co-authored-by: erialC-P <[email protected]>
Closing PR. New PR initiated due to complicated conflicts with develop. |
Proposed changes
New notebook.
Concerns
Kernel crashes when user requests a large AOI and a large number of images are extracted. Mostly occurs in the Baseline load_ard extract. Not sure if I can use Dask to solve as I need to visualise the images in the following plot.
Checklist (replace
[ ]
with[x]
to check off)Notebook created using the DEA-notebooks template
Remove any unused Python packages from
Load packages
Remove any unused/empty code cells
Remove any guidance cells (e.g.
General advice
)Ensure that all code cells follow the PEP8 standard for code. The
jupyterlab_code_formatter
tool can be used to format code cells to a consistent style: select each code cell, then clickEdit
and then one of theApply X Formatter
options (YAPF
orBlack
are recommended).Include relevant tags in the final notebook cell (refer to the DEA Tags Index, and re-use tags if possible)
Clear all outputs, run notebook from start to finish, and save the notebook in the state where all cells have been sequentially evaluated
If applicable, update the
Notebook currently compatible with the NCI|DEA Sandbox environment only
line below the notebook title to reflect the environments the notebook is compatible withTest notebook on both the
NCI
andDEA Sandbox
(flag if not working as part of PR and ask for help to solve if needed)