Review

[vjugor1]

README.md

• Please be more specific: "some data analysis" -> "plots distributions of climate variables and distribution of climate features condition on class - crop/non-crop land"
• "diverse .py" -> diverse.py

Markdown

• Add more comment in the in the markdown. At least outline the structure with them. A good example of notebook and use of markdowns can be found here, however, it has too much comments, it is not necessary to have such storytelling in this project.

Dockerfile

Please wrap all your code into docker container

• Here is an example of Dockerfile
• You should make your requirements.txt -- see here
• An example of README.md with docker instructions
• Why one needs it? To be able to run the code just by building the docker image (1 command in terminal) + starting a container (1 command in terminal) -- that saves a user headache with package installation + docker container is a common industry practice

notebook_crop.ipynb

• Please delete long outputs, like in cell 10
• cell 3 - wrap it into one function and put it into some .py file (incapsulate)
• cell 5 - incapsulate it
• cell 8 - what is this strange parallelogram in the bottom?
• cell 5 who is it?
• Increase the font sizes in plots so it would be comparable to the text in the paper
• Aside from that, it is very good!

notebook_yield.ipynb

• Better to make fertilizer forecasts from regression models continuous with the historical (fix this gap, see screen below)
• Or delete regression from this notebook, if you do not use it anyway (but keep it somewhere else)
• It would be good to write conclusion and a bit of analysis below the final table

Data

• Please provide link to the data, e.g., for google drive and guidelines where to put the data downloaded.

'.pkl' files

• There is a commented cell in notebook_yield.ipynb, mention it in README.md

[vjugor1]

Don't forget to delete old commented code (like here), or store it somewhere else

[vjugor1]

Another minor comment, within src.utils keep to the same name convention: if you use variable name clf in one function, do not use variable name model for the same substance in another place

[vjugor1]

src.utils docstring for this function looks outdates (missing one argument). This makes the unclear what does out_meta.update method

Stopped here, will continue

[vjugor1]

• crop_country does not return anything, however, docstring stats that it should return 3 .tifs. Seems like this function saves the results to a file. Please, change the dostring. The same with crop_raster

• yield_rolling_mean I would rather divide split it into two. Something like 1 - read_faostat_rice_production, 2 - prepare_rolling_mean Here, when you do plt.show(), it is more practical to omit it. By calling this method, you finalize and close to edit this figure. That might be inconvenient if you want to further update it in notebook.

• collect_data_yield

• Magical constant 5, please infer 5 from some argument

• Why years[1] here?

• Seems like [const_2] (https://github.com/DariaTan/ArableLand2/blob/479cda1a21ce3570db363f79b0b08a2835d3ec5a/src/utils.py#L498) is redundant to have inside the nested loop. Moreover, considering the definition of const_array_0, const_array_1, const_array_2, it is not clear why are they under loop over prefix, since, for each iteration, you assemble them over the full set of prefixes.

-Please, change the loop variable name prefix or do not use variable name prefix inside list comprehension in this loop, e.g., here. It is 1. misleading, 2. dangerous - something can be overwritten

• Please rewrite this function fully, considering the advises above. At best, this function will be more time and resource efficient, at worst, we will locale a bug here. We can discuss it if you want, I might be wrong on some points.

• The same applied for collect_future_data

• Stopped at this

[DariaTan]

• Why years[1] here? It happens because rolling mean procedure turns first and last values to nan. We should kinda drop them

• Seems like [const_2] ) is redundant to have inside the nested loop. Moreover, considering the definition of const_array_0, const_array_1, const_array_2, it is not clear why are they under loop over prefix, since, for each iteration, you assemble them over the full set of prefixes.

Well, I can believe it looks ugly a bit. It is literally one hot encoding done by hand. Arrays are defined inside the loop since they are set of zeros. When we use them as features for object, you put 1 to the column related with the country (prefix here)

[vjugor1]

src.utils

• neg_trend It is not quite quite clear who is neg_trend. Could you please elaborate on it in docstring or in a comment above the usage of it in the code?
• '2018' It is better to move '2018' here and here to the arguments of the function. It will make the code a) more flexible; b) clearer

src.models.crop_classifier.py

• docstrings Not sure that 2d numpy array is callable, like it is stated here. Do you use some docstring generator? Also, the type of '2d numpy array' is np.ndarray (provided you have imported numpy as follows: import numpy as np. As a convention, number of dimensions in this array should be mentioned after ':'. By the way, you can state the argument types in the definition of function, e.g., def foo(x: int, y: int):. In this case, docstring generators will automatically infer the information about the arguments type and mention it in the docstring in a proper way.
• params_xgb It better to pass this as an argument of this function. You can make this parameters to be present by default, thus, you won't need change anything in you code further.
• params_skf - similar comment
• params_gs - similar comment
• classifier.model This methods looks like a constructor, consider erasing this one and moving the functionality into the __init__.
• Nice class!

  src.models.fert_regression.py

• I would rename this function into train_pred_future_fert and changed the docstring accordingly. At the moment of posting this, it says that this function create a model for each years, thus the name of the function and the docstring is misleading.
• font sizes For ergonomics, consider moving fontsize from hardcoded 14 here to the arguments of the model function

I am moving to notebooks