small fixes in input for calculate_indices() rgb()

[GL-S]

Proposed changes

added line to prevent warnings from dask.
Changed collection name in calculate_indices(), and reset band names in rgb() to default to avoid errors

Closes issues (optional)

• Closes Issue Scalable Machine Learning workflow - fails  #1271

Checklist

(Replace [ ] with [x] to check off)

• Notebook created using the DEA-notebooks template
• Clear all outputs, run notebook from start to finish, and save the notebook in the state where all cells have been sequentially evaluated
• Test notebook on both the NCI and DEA Sandbox (flag if not working as part of PR and ask for help to solve if needed)

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[JM-GA]

@GL-S - might need to revert this change - guessing it was the one testing git access

[GL-S]

Yes you are correct, I will

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

robbibt commented on 2025-01-16T05:19:56Z
----------------------------------------------------------------

Line #19.    dask.config.set({'distributed.worker.daemon': False}); # Prevent some warnings from being printed

Can you elaborate on this - e.g. what warnings? We could potentially move this to the dea_tools.dask module itself if it seems like a good idea, but I'm not exactly sure what this code does (just want to make sure we don't accidently break anything by adding it)

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

robbibt commented on 2025-01-16T05:19:57Z
----------------------------------------------------------------

Line #7.        ds = dc.load(product='ga_ls8cls9c_gm_cyear_3', 

Yay, great fix - we must have missed this last year

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

robbibt commented on 2025-01-16T05:19:57Z
----------------------------------------------------------------

Line #7.            # default band names: ['nbart_red', 'nbart_green', 'nbart_blue']

I reckon we can probably remove this line if this func now works directly on bands with those names

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

robbibt commented on 2025-01-16T05:19:58Z
----------------------------------------------------------------

Line #6.        # default band names: ['nbart_red', 'nbart_green', 'nbart_blue']

Same here; I'd probably remove this line just to keep things simple

[robbibt]

This looks great, thankls @GL-S! I left a few minor comments on the notebook... for the Dask thing, my main priority is just making sure the new line doesn't accidently cause any other downstream issues. If we're sure it doesn't, then we could consider moving it from this notebook to the dea_tools.dask module instead to hide it from external users?

[GL-S]

Thank you for the comments @robbibt .
The warnings appeared when running the model for classifying the satellite images, there is a lot of printing which doesn't look great.
I added the line about the dask's warnings because it was a solution suggested by the warning itself (screenshot added below). I am actually not sure what it does in details, it should have an influence on how the processes run and exit. It's probably safer not to add it to dea_tools.dask yet.
...maybe a filterwarnings in dea_tools.dask is a better solution? It doesn't work when added to the 4_Classify_satellite_data.ipynb

Regarding the comments about the band names, I get your point. I will remove those commented lines

[robbibt]

Thanks @GL-S! Yeah, those warnings look terrible, it would definitely be nice to hide them away.

@emmaai Do you know what these Dask warnings in the screenshot above mean, and whether adding dask.config.set({'distributed.worker.daemon': False}) to our dea_tools.dask script would be a safe move?

[emmaai]

The warning seems to suggest there was a conflict between config worker as daemon and number of jobs = 1. Intuitively I don’t know why it can’t be both. The suggested solution is setting worker process as non-daemon. I don’t think it’s a good idea in general. Also as it runs with scikit-learn, I suspected the warnings actually suggested something different and more serious. Scikit-learn is multi-threading, it’s tricky to run with dask. I didn’t read the notebook. If it couldn’t be understood and resolved with reasonable thought, I’d suggest to keep it until further investigation.

[GL-S]

Hi all, I have submitted another commit.
I removed the dask.config.set({'distributed.worker.daemon': False}) to be safe, as Emma mentioned that it might not be a good idea. I will follow up on this when the notebooks COP resumes, as James suggested.
I have also removed the commented lines about the default band names.
Effectively, the changes from the version in develop are the updated product and collection names in the function feature_layers. The warnings are still there, but at least the notebook runs with no errors.

[robbibt]

Thanks heaps @GL-S, I think this is a good approach for now. Any chance you could create quick Github issue in this repo describing the warnings issue, that we can refer to later to work out what's going wrong?

Other than that, I think this is ready to "Squash and merge" into the develop branch 🙂

[GL-S]

Sure, I will create an issue now