Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding new years to a MESSAGE scenario #161

Merged
merged 49 commits into from
Jun 21, 2019
Merged

Conversation

behnam-zakeri
Copy link
Contributor

@behnam-zakeri behnam-zakeri commented Feb 4, 2019

PR checklist

  • Tests added.
  • Documentation added.
  • Description in RELEASE_NOTES.md added.

@OFR-IIASA
Copy link
Contributor

@behnam2015 please review scripts to make these pep8 conform. Once this is done, you will see that stickler-ci tests pass.

@OFR-IIASA
Copy link
Contributor

@behnam2015 If I understand correctly, then this script essentially makes a new scenario from an existing scenario by copying all individual sets and parameters. In my opinion, that seems like a large workload as opposed to just adding datapoints, where required to an existing scenario; and should also save a lot of time in terms of processing and probably also reduce the number of lines of code. Also, as opposed to having this as a standalone tool, I think it would be desirable to have this part of the message_ix class, so that you can simply call e.g. "scen.toolbox.add_year([2025])". I also think that changing the firstmodelyear or changing the macro initial year shouldn't be part of the script's job. The script should be limited to simply adding additional datapoints. @gidden and @danielhuppmann would you agree? Are there any technical specifics that wouldn't allow this?

@OFR-IIASA
Copy link
Contributor

Talking to Daniel it seems that due to the implementation in gams and java, it is not possible to insert a year into an existing model. Everything needs to be defined from the beginning, as otherwise the new year being added would end up being inserted after the last year, as both of the above dont know about the correct sequence of years.

@behnam-zakeri
Copy link
Contributor Author

@OFR-IIASA thank you for the feedback and evaluating the code. There was one pep8 issue I resolved it, hopefully it passes the test now.

@behnam-zakeri
Copy link
Contributor Author

@OFR-IIASA as you mentioned, the year sequence was the reason that the new years are added to an empty scenario and all parameters are then copied from the reference scenario to the new one.

@gidden gidden requested a review from khaeru April 29, 2019 11:37
@gidden
Copy link
Member

gidden commented Apr 29, 2019

would you mind taking a look here @khaeru? I will dig into #190

RELEASE_NOTES.md Outdated Show resolved Hide resolved
Copy link
Member

@khaeru khaeru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be a solid addition! I am adding some notes here, in advance of a meeting on 2019-05-02 to discuss how to execute them:

  • Use click rather than argparse for command-line interface & argument handling.
  • Follow the PEP8 prescriptive naming conventions, in particular:
    • remove leading f_ from method names.
  • Dissolve the class addNewYear to individual methods. Searching the code for self. shows that the class instance, self, is only used to refer to other method names. In this case, the class doesn't add anything (except indentation 😁).
  • Use message_ix.tools instead of message_ix.toolbox namespace.
  • Link the README for the new tool into the documentation build.

@behnam-zakeri
Copy link
Contributor Author

Thanks a lot @khaeru for the comments and for your great additions! The use of click for comman-line handling looks very good and concise. I reformatted the class to a function, now called add_year. Originally this was a class to reduce the number of arguments passed over different functions, but it seems it's not needed anymore.

@khaeru khaeru self-requested a review May 8, 2019 12:45
Copy link
Member

@khaeru khaeru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the effort to tidy this up @behnam2015! It's a solid improvement to MESSAGEix that will be useful for many.

@gidden, since you requested my review, did you want to give this a second review?

Copy link
Member

@gidden gidden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi folks, happy to approve as-is. There are a few housecleaning things I would request here:

  1. This introduces a new "standard" for calling CLIs (namely via the -m option). My preference here would be we choose a standard and stick to it. See e.g., setup.py:
   entry_points = {
        'console_scripts': [
            'messageix-config=message_ix.cli:config',
            'messageix-dl=message_ix.cli:dl',
        ],
}

I have no strong preference between the options here, but would suggest we maintain consistency. Let me know what you think!

  1. Can you please add an issue to https://github.com/conda-forge/message-ix-feedstock/ to also test this CLI in the feedstock?

RELEASE_NOTES.md Outdated Show resolved Hide resolved
@khaeru
Copy link
Member

khaeru commented Jun 14, 2019

  1. This introduces a new "standard" for calling CLIs (namely via the -m option). My preference here would be we choose a standard and stick to it. See e.g., setup.py:
   entry_points = {
        'console_scripts': [
            'messageix-config=message_ix.cli:config',
            'messageix-dl=message_ix.cli:dl',
        ],
}

I have no strong preference between the options here, but would suggest we maintain consistency. Let me know what you think!

@behnam2015 and I used click in this PR to have a tidy CLI; but replacing the existing CLI was out of scope.

I think it should be a target for a future release to have a single entry point (message-ix) and then use click to dispatch to different commands. Once the overall CLI uses click, then the add_year tool/CLI can be converted to just one of the commands, and it can be called with message-ix add_year [options] [args]. For the moment, IMO it makes sense to keep it separate.

message_ix/tools/add_year/__init__.py Outdated Show resolved Hide resolved
message_ix/tools/add_year/__init__.py Outdated Show resolved Hide resolved
message_ix/tools/add_year/__init__.py Outdated Show resolved Hide resolved
message_ix/tools/add_year/__init__.py Outdated Show resolved Hide resolved
message_ix/tools/add_year/__init__.py Outdated Show resolved Hide resolved
message_ix/tools/add_year/__init__.py Outdated Show resolved Hide resolved
@khaeru khaeru added this to the 1.2 milestone Jun 21, 2019
@khaeru khaeru added the enh New features & functionality label Jun 21, 2019
@gidden
Copy link
Member

gidden commented Jun 21, 2019

Ok, great advance here! Thanks all who helped out!

@gidden gidden merged commit 5d7e321 into iiasa:master Jun 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enh New features & functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants