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

Forecasting/modelling notebook #911

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

Forecasting/modelling notebook #911

wants to merge 9 commits into from

Conversation

cbur24
Copy link
Collaborator

@cbur24 cbur24 commented Mar 31, 2022

Proposed changes

Proposing to add a vegetation forecasting/modelling notebook to the Real_world_examples folder, ported over from DE Africa repo.

In draft form until this issue is resolved

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@robbibt
Copy link
Member

robbibt commented Jan 9, 2023

Hey @cbur24! Just checking on on this one - was this previously blocked by issues with the NCI environment?

@cbur24
Copy link
Collaborator Author

cbur24 commented Jan 9, 2023

@robbibt This PR was in draft form as the Sandbox image needed to be updated due to a conflict between scipy and statsmodel.
I've rerun the notebook and can confirm it works now. I'll label it as ready-for-review and leave it up to the DEA team to decide if its something they want in the repo.

@cbur24 cbur24 marked this pull request as ready for review January 9, 2023 04:42
Copy link
Member

@uchchwhash uchchwhash left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Member

@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.

Hey @cbur24, sorry this has taken so long for us to progress! It's a really great notebook, and will be awesome to finally get it merged in.

Since the notebook was written, there's been a bunch of changes including Sentinel-2 products being renamed. I've checked out the branch and updated them to the new Collection 3.

The notebook was also pretty slow on the default Sandbox server, so I've updated the notebook to use only Sentinel-2A data, and a min_gooddata of 0.9. This means a lot less timesteps to load, but may potentially have changed the results - could you check the new outputs and make sure everything is as expected?

My only requested change is around the intro of the notebook: the "Background" section dives straight into pretty technical detail right up. I think adding in a sentence or two at the very beginning outlining the scientific/management case for forecasting using remote sensing data would make the notebook much more approachable to beginner users: essentially, outlining the case for why it's an important tool/case study. 🙂

Apart from that, looks great to me!

@cbur24
Copy link
Collaborator Author

cbur24 commented Oct 23, 2023

@uchchwhash @robbibt
Thanks guys. I had a look at this notebook again last week during some down time which is why I pushed the re-merge with develop. I'll implement the changes requested next time I've got an hour or two free.

@robbibt
Copy link
Member

robbibt commented Nov 22, 2023

Hey @cbur24, would love to merge this - I'm happy to write up a sentence or two for the intro if you're busy - I just wanted to add a tiny bit extra to emphasise the importance/usefulness of the use case. 🙂

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.

3 participants