search

OMS-NetZero / FAIR

Finite-amplitude Impulse Response simple climate model
https://docs.fairmodel.net
Apache License 2.0
121 stars 61 forks source link

Created a numpy version of the concentration driven mode #86

Closed JGBroadbent closed 3 years ago

JGBroadbent commented 3 years ago

Same formatting as emissions driven mode and tests written

Pull request

Please confirm that this pull request has done the following:

Adding to CHANGELOG.rst

Please add a single line in the changelog notes similar to one of the following:

- (`#XX <http://link-to-pr.com>`_) Added feature which does something
- (`#XX <http://link-to-pr.com>`_) Fixed bug identified in (`#XX <http://link-to-issue.com>`_)
JGBroadbent commented 3 years ago

Hi Zeb,

I've not run any tests yet, just wanted to get your input before proceeding any further. RE the repeated code, I completely agree, there's a few repetitions of combinations of functions but mostly the whole thing is run out of common functions anyway.

JGBroadbent commented 3 years ago

Yep, so I wanted to make sure that the input into the 'numpy version' was always properly formatted, but also I think it's not very user friendly to require people to enter the parameters, gasses etc all in the right order (or indeed in time order), so it makes sense to just sort everything by column and by index before proceeding. RE the openSCM runner, my impression is that I can fairly easily "bolt" that on top of this? Once I've got this to a desired standard I'll write a first draft of that, then come back and test everything.

znicholls commented 3 years ago

Yep, so I wanted to make sure that the input into the 'numpy version' was always properly formatted, but also I think it's not very user friendly to require people to enter the parameters, gasses etc all in the right order (or indeed in time order), so it makes sense to just sort everything by column and by index before proceeding

Cool that works. As we try and add more stuff we'll get a better sense of what the best system is so let's not sweat too much now.

RE the openSCM runner, my impression is that I can fairly easily "bolt" that on top of this?

Yep. Let's get all the tests passing here and we're happy with our pandas run interface, bolting on openscm runner should be pretty simple.

JGBroadbent commented 3 years ago

Great, will jump on that.

znicholls commented 3 years ago

nice @JGBroadbent, my suggested next steps would be:

  1. rebase and remove the tests/pytest-env folder which is now in the repo. We don't want to be including virtual environments (and the 3 000 files they contain) in our repo!
  2. merge this
  3. I'll make a follow up merge request which adds continuous integration to our setup here (so the tests get run by github on pull requests as a final check)
  4. then you make merge requests which add extra functionality (with the tests making sure we don't regress i.e. break anything in the process of our upgrades)
  5. rinse and repeat, including other tests to add docs, linting etc. (I'll do these but I'll be a bit slow so let's not hold everything up) in the process
JGBroadbent commented 3 years ago

I thought I’d excluded the tests/pytest! oh well, will do!

On 30 Sep 2020, at 10:15, Zeb Nicholls notifications@github.com wrote:

nice @JGBroadbent https://github.com/JGBroadbent, my suggested next steps would be:

rebase and remove the tests/pytest-env folder which is now in the repo. We don't want to be including virtual environments in our repo! merge this I'll make a follow up merge request which adds continuous integration to our setup here (so the tests get run by github on pull requests as a final check) then you make merge requests which add extra functionality (with the tests making sure we don't regress i.e. break anything in the process of our upgrades) rinse and repeat, including other tests to add docs, linting etc. (I'll do these but I'll be a bit slow so let's not hold everything up) in the process — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/OMS-NetZero/FAIR/pull/86#issuecomment-701268712, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQR67PFTBL26SRSILXVVUQDSILZLRANCNFSM4RW3BGFA.

JGBroadbent commented 3 years ago

Done

On 30 Sep 2020, at 10:16, John Broadbent johngeoffreybroadbent@gmail.com wrote:

I thought I’d excluded the tests/pytest! oh well, will do!

On 30 Sep 2020, at 10:15, Zeb Nicholls <notifications@github.com mailto:notifications@github.com> wrote:

nice @JGBroadbent https://github.com/JGBroadbent, my suggested next steps would be:

rebase and remove the tests/pytest-env folder which is now in the repo. We don't want to be including virtual environments in our repo! merge this I'll make a follow up merge request which adds continuous integration to our setup here (so the tests get run by github on pull requests as a final check) then you make merge requests which add extra functionality (with the tests making sure we don't regress i.e. break anything in the process of our upgrades) rinse and repeat, including other tests to add docs, linting etc. (I'll do these but I'll be a bit slow so let's not hold everything up) in the process — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/OMS-NetZero/FAIR/pull/86#issuecomment-701268712, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQR67PFTBL26SRSILXVVUQDSILZLRANCNFSM4RW3BGFA.