Skip to content

Crop suitability #280

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

Merged
merged 75 commits into from
Jul 20, 2023
Merged

Crop suitability #280

merged 75 commits into from
Jul 20, 2023

Conversation

drewmresnick
Copy link

@drewmresnick drewmresnick commented Jan 27, 2023

Maproom for crop suitability analysis.

To run, navigate to inside of the crop_suitability folder and run the following command:
CONFIG=../config.yaml:../config-nma.yaml:../myconfig.yaml python maproom_crop_suit.py

Current issues:

  1. Slow speed to calculate crop suitability

  2. Trouble with figure out the best way to count number of dry spells in the calculation for crop suitability

  3. Bug in selecting season for DJF (selecting all months of same year instead of the december from the previous year)

@drewmresnick drewmresnick reopened this Feb 21, 2023
@drewmresnick drewmresnick marked this pull request as draft February 21, 2023 16:16
@drewmresnick drewmresnick requested a review from kgraaf February 21, 2023 16:16
@drewmresnick drewmresnick marked this pull request as ready for review March 31, 2023 14:47
Copy link
Collaborator

@aaron-kaplan aaron-kaplan left a comment

Choose a reason for hiding this comment

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

Not a thorough review, but here are some comments to get you started.

As I said by email, you should check out the dry spell functionality in the onset maproom. It's not exactly what you need, but I think it will help.

@remicousin
Copy link
Contributor

For dry spells defined as Tufa did, there is longest_run_length that could be a source of inspiration. Instead of taking the max at the end, could divide by the length that defines a dry spell, that would give a count of spells within a series of continuous spells, and then sum that to count all the spells in the season. I think... need to check what goes into the max is what I claim here, it's been a while...
Dividing (or using modulo) should be done on integers so that, if the threshold for spells is 5 days, 6 will return 1, 14 will return 2, etc.. which sounds appropriate.
Now there could be an approach outside longest_run_length that is better but I'd have to think more about it.

@remicousin
Copy link
Contributor

Is the analysis meant to be performed on a single year chosen by the user? Or will there be analysis through years at some point (either some reduction to make map or some repetition to make time series)? Asking for a friend. :) I mean: asking to think thoroughly of the dry spell problem.

@drewmresnick
Copy link
Author

Is the analysis meant to be performed on a single year chosen by the user? Or will there be analysis through years at some point (either some reduction to make map or some repetition to make time series)? Asking for a friend. :) I mean: asking to think thoroughly of the dry spell problem.

@remicousin As of now the crop suitability analysis is used both to generate the map for a single year and a time series plot over all available years of data. So I am trying to build it so that it can work both for a single year and for multiple years.

@remicousin
Copy link
Contributor

As a general comment, can we name the app similarly to the CSMT Maproom? I think the inspiration comes from there. So something like Climate Suitability fro Crop ... something-something.

Copy link
Contributor

@remicousin remicousin left a comment

Choose a reason for hiding this comment

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

I did a first review. Key things:

  • what seasons do we want as options;
  • consider my proposition on how to select seasons/years
  • note that when rebasing, the colorscales won't work as the new system has merged
  • consider leaving the dry spell component to another PR to limit scope of this PR
  • I would like to review more when I am able to build

@drewmresnick
Copy link
Author

@remicousin I just pushed the changes so that you should be able to build the maproom by running
CONFIG=../config.yaml:../config-nmal:../myconfig.yaml python maproom_crop_suit.py

Where myconfig.yaml just has your db password. Note it is slow so it will take a minute to load the map and time series plot.

Going back now to look at the additional comments and will respond to them / push additional commits as it appropriate.

@drewmresnick
Copy link
Author

As a general comment, can we name the app similarly to the CSMT Maproom? I think the inspiration comes from there. So something like Climate Suitability fro Crop ... something-something.

As simple as: "Seasonal Climate Suitability for Crops in Ethiopia" ?

@remicousin
Copy link
Contributor

@remicousin I just pushed the changes so that you should be able to build the maproom by running CONFIG=../config.yaml:../config-nmal:../myconfig.yaml python maproom_crop_suit.py

Where myconfig.yaml just has your db password. Note it is slow so it will take a minute to load the map and time series plot.

Going back now to look at the additional comments and will respond to them / push additional commits as it appropriate.

It builds without failing but I am not sure how you set this up but it is so that I can't change the port and hostname so basically devi:8050 so I won't do that because you might be using it for testing purposes as you work.

But I think we are missing something from @aaron-kaplan to build individual pages again.

@remicousin
Copy link
Contributor

As a general comment, can we name the app similarly to the CSMT Maproom? I think the inspiration comes from there. So something like Climate Suitability fro Crop ... something-something.

As simple as: "Seasonal Climate Suitability for Crops in Ethiopia" ?

Maybe just Climate Suitability for Crops. I would remove seasonal because CSMT does make a climatological analysis and shows indeed the seasonal cycle only. Here we are going to show year-to-year variability as well. You can add "in Ethiopia" but that part would have to be dealt with through the national config file. I typically would not add it because the app has a vocation to be part on a website dedicated to Ethiopia, so there is no need to add Ethiopia everywhere in titles or so.

@drewmresnick
Copy link
Author

@remicousin I just pushed the changes so that you should be able to build the maproom by running CONFIG=../config.yaml:../config-nmal:../myconfig.yaml python maproom_crop_suit.py
Where myconfig.yaml just has your db password. Note it is slow so it will take a minute to load the map and time series plot.
Going back now to look at the additional comments and will respond to them / push additional commits as it appropriate.

It builds without failing but I am not sure how you set this up but it is so that I can't change the port and hostname so basically devi:8050 so I won't do that because you might be using it for testing purposes as you work.

But I think we are missing something from @aaron-kaplan to build individual pages again.

I copied the water balance maproom since that is the other standalone maproom. I am not sure about why you would not be able to change the port and hostname.. I had not tried to do so though I have been working on shortfin if that makes any difference to you.

@aaron-kaplan
Copy link
Collaborator

It builds without failing but I am not sure how you set this up but it is so that I can't change the port and hostname so basically devi:8050 so I won't do that because you might be using it for testing purposes as you work.

In case it wasn't clear, that was the reason for this comment.

But I think we are missing something from @aaron-kaplan to build individual pages again.

I started working on that, but a lot of other things have taken priority. I think the way things are set up here is workable for now, just need the right parameters in the run command.

@drewmresnick
Copy link
Author

The remaining tasks to do are:

  1. Improve speed (I have a meeting with Aaron today to discuss this).
  2. Add tests for suitability function (move this function to a different file?)
  3. Fix / improve season def
  4. Add / fix parameters in suitability function:
    • Maximum season length
    • Minimum number of wet days in a season
    • maximum number of dry spells in first x months of season
      I recieved a clarification email from Tufa regarding the seasons def, "the length of the season is the duration between onset and cessation, which are defined by the onset and cessation parameters"
      So actually now I think that the season is not a start date and length defined by the user, but actually parameters for onset and cessation dates which are used as the start and end dates of the rainy season. I replied to Tufa to confirm this though.

As Rémi and I discussed, most of this will be done in separate PR's. I will try to fix the loading speed of the current state before merging this branch, and other smaller fixes but then to deal with the additional parameters in the suitability function and the fixing of the season def, I will open new PR's. Sound good?

@remicousin remicousin merged commit ca0431d into master Jul 20, 2023
@remicousin remicousin deleted the crop_suitability branch July 20, 2023 14:29
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