Conversation
| data=Exposures.concat(bkmrbl_list).gdf, | ||
| crs=DEF_CRS, | ||
| ref_year=ref_year, | ||
| tag=Tag(file_name=fn_nl, description=description), |
There was a problem hiding this comment.
Also in line 36 above we can remove the import.
chahank
left a comment
chahank
left a comment
There was a problem hiding this comment.
Small comments only, otherwise good, thanks!
| self, | ||
| data=Exposures.concat(gdp2a_list).gdf, | ||
| ref_year=ref_year, | ||
| tag=Tag(description=description), |
There was a problem hiding this comment.
The tag import in line 31 can be remove I think.
There was a problem hiding this comment.
Thanks for pointing this out!
requirements/env_docs.yml
Outdated
| - ipython | ||
| - ipykernel | ||
| - mccabe>=0.6 | ||
| - nb_conda |
There was a problem hiding this comment.
This is a bit unrelated to this PR imho. Why is this necessary for the tag removal?
There was a problem hiding this comment.
Right, this is only for making readthedocs run again. It's now in develop as well.
to prevent meta data loss by tag removal
|
Eventually it turned out to be a bit more complicated than hoped for. Three exposures classes in petals actually do store information about how the dataset was constructed in the tag's description. This information is shown in the exposures plots and to me doesn't see to be completely useless. Furthermore the datasets from the API seem to have contain information in the tags that I'm not sure is available otherwise. Besides I also kept the raw data files for 🤷 |
|
I think we should get rid of these description elements unless we are able to make a stable and useful solution. The information in the plots are almost always not necessary. Concerning the data API, I would say it is a bad practice if relevant information is hidden in the attribute of a file instead of being part of the metadata of the file in the database. Thus, I would again argue for removing the description rather than keeping it. |
|
As far as I'm concerned I agree, but whether something is useful or not is in the eye of the beholder. Concerning the data API: fully agreed that is bad practice. But in case it was ever applied, we would need to replace the concerned datasets in a rather elaborate procedure that involves the On the other side: having a |
peanutfun
left a comment
peanutfun
left a comment
There was a problem hiding this comment.
We should make description a "regular" attribute of the exposure classes that use it, and document it accordingly. Other than that, all looks fine.
Thank you for improving the tests by using better asserts than assertTrue, this is much better! 🙌
| exp.description=("Crop production exposure from ISIMIP" | ||
| f" {CROP_NAME[crop]['print']} {irr}" | ||
| f" {yearrange[0]} {yearrange[-1]}") |
There was a problem hiding this comment.
Similar topic: I find it bad practise to set an attribute that is not defined in the constructor. (I know that Exposures combines several bad practices when it comes to constructors but we don't have to make it worse)
The description attribute is also never documented anywhere.
Co-authored-by: Lukas Riedel <[email protected]>
3 participants
Changes proposed in this PR:
This PR fixes #
PR Author Checklist
develop)PR Reviewer Checklist