remove tag from Exposures

CLIMADA-project / climada_petals

See https://github.com/CLIMADA-project/climada_python first

GNU General Public License v3.0

22 stars 13 forks source link

remove tag from Exposures #89

Closed emanuel-schmid closed 10 months ago

emanuel-schmid commented 11 months ago

Changes proposed in this PR:

Remove all tag occurrences from exposures classes

This PR fixes #

PR Author Checklist

[ ] Read the Contribution Guide
[x] Correct target branch selected (if unsure, select develop)
[x] Descriptive pull request title added
[x] Source branch up-to-date with target branch
[ ] Documentation updated
[x] Tests updated
[ ] Tests passing
[ ] No new linter issues
[ ] Changelog updated

PR Reviewer Checklist

[ ] Read the Contribution Guide
[ ] CLIMADA Reviewer Checklist passed
[ ] Tests passing
[ ] No new linter issues

emanuel-schmid commented 11 months ago

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. So I decided to keep the description - for the time being - as an attribute, not just for LitPop but for all Exposures classes.

Besides I also kept the raw data files for BlackMarble and SpamAgrar, not as file_name but as nightlight_file and spam_file respectively, because the identification of the suitable file is part of the creation process and not provided by the user.

🤷

chahank commented 11 months ago

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.

emanuel-schmid commented 11 months ago

As far as I'm concerned I agree, but whether something is useful or not is in the eye of the beholder. You say the information in the plots is almost always not necessary - sounds to me like exceptionally they may benecessary.

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 api_client.Client from climada 3.x - or rebuild them from scratch.

On the other side: having a description attribute in the Exposures class looks quite harmless to me. There is an arbitrary string in the class obviously containing a description of the object - so what? And if it shows up in plots - all the better! Is it useful? s.a. Is it stable? Stable enough I'd say.