Added save_csv_combined_output option

[gabriel-abrahao]

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

• Tests added
• Documentation added
• Example added (in the documentation, to an existing notebook, or in a new notebook)
• Description in CHANGELOG.rst added (single line such as: (`#XX <https://github.com/iiasa/climate-assessment/pull/XX>`_) Added feature which does something)

[jkikstra]

Hi @gabriel-abrahao, that's great to hear you'll be contributing!

Making a PR like this works well for me.
And this looks like a straightforward addition.

The only thing I can see is that we're (for the first time) adding an option to clim_cli() which is not passed on to climate_assessment().
@phackstock and @znicholls any objections or thoughts from your side?

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).

[jkikstra]

maybe call it save_csv_combined_climate_output_option to be a bit more descriptive (it does not include emissions data, right?) and align better with save_raw_climate_output_option

[gabriel-abrahao]

It does include emissions, it's the rawoutput file. That's why I went for "combined", but suggestions on a more descriptive name are appreciated

[jkikstra]

Ah yes you're right.

Okay, rather then thinking about it too long, I think what can simply do is add a bit more description in the docs: #44
Can be done after we merge this PR.

[znicholls]

All fine for me (will write a longer comment below).

Can you please add at least one test of this.

[znicholls]

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]

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

[phackstock]

No objections from my side.

[gabriel-abrahao]

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.

[znicholls]

lgtm, some minor clarifications which I think would help

[znicholls]

lgtm, assuming CI passes let's merge