iiasa / climate-assessment

https://climate-assessment.readthedocs.io/en/latest
MIT License
19 stars 18 forks source link

Added save_csv_combined_output option #43

Closed gabriel-abrahao closed 1 year ago

gabriel-abrahao commented 1 year ago

Hi all,

We at PIK have started integrating climate-assessment in some of our workflows, such as in the REMIND model. For that, adding some minor new features to the package would be really helpful, which I aim to implement. I don't know what's your policy for that, but in case PRs are the proper channel, here's one.

It simply adds an option in clim_cli to also write raw output as CSV besides the XLSX standard. Reading excel can be pretty cumbersome for some of our applications.

All tests in tests/integration have passed, is this sufficient for a PR?

Thank you regardless, and please let me know if I should be using another channel for this, Gabriel

znicholls commented 1 year ago

We at PIK have started integrating climate-assessment in some of our workflows, such as in the REMIND model. For that, adding some minor new features to the package would be really helpful, which I aim to implement

This is great, but also a bit scary, mainly because I would like to re-write lots of this over the coming months/years so please don't get too attached to the idea that this is a stable interface. We will try and use deprecation warnings etc., but there's lots of things we could have done better/differently which may require us to break the current API.

All tests in tests/integration have passed, is this sufficient for a PR?

We don't really have a contributing guide yet so will be good enough for now. If we change that, we'll let you know. As I said above, if you could add a test that would be great.

For in the future, if you're planning any larger modifications, it might be good to have a discussion before starting to discuss the design (either in a call, or on GitHub through an Issue or Discussion).

This is good advice. I'm happy with issues unless it is huge, in which case a call is helpful (but I can't think of any cases where that is needed).

jkikstra commented 1 year ago

All good notes and heads up by @znicholls.

P.S. to satisfy the linters, see: https://climate-assessment.readthedocs.io/en/latest/dev.html#formatting-code

black --check src scripts tests setup.py
isort --check-only --quiet src scripts tests setup.py
flake8 src scripts tests setup.py
gabriel-abrahao commented 1 year ago

Thank you all!

This is great, but also a bit scary, mainly because I would like to re-write lots of this over the coming months/years so please don't get too attached to the idea that this is a stable interface. We will try and use deprecation warnings etc., but there's lots of things we could have done better/differently which may require us to break the current API.

Sure, we are aware of that, and we are (maybe a bit too much) used to freezing to a very old versions if it comes to that in the future. I'm also trying to always invoke the scripts in production setups and avoiding the actual API as much as possible. Not sure if that helps stability.

Can you please add at least one test of this.

I added a test that checks if both the XLS and the CSV rawoutput files are written if clim_cli is run with the new option. Not sure if that's really an integration test, so let me know if it's in the wrong place. Also did some changes to satisfy the linters.

For in the future, if you're planning any larger modifications, it might be good to have a discussion before starting to discuss the design (either in a call, or on GitHub through an Issue or Discussion).

Thanks, I'll absolutely do that. But I'm not really expecting to need anything much more complex than this. One other thing that could be useful would be exposing some internal functions (both from here and from the other SCM-related packages), but I'll ask as an issue in the relevant packages if it comes to that.