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

build core-cge-ml module #473

Merged
merged 12 commits into from
Apr 4, 2024
Merged

Conversation

Rashmil-1999
Copy link
Contributor

In this PR I have added a new module called the Core CGE ML where the idea is to implement all repetitive actions related to CGE ML here. Since loads of models will come in, we will create a wrapper class for that testbed and keep model coefficients, base values, and the related files in that analysis folder. The core CGE ML will be imported, fed the data it needs, and spit out the outputs. That will be the goal of all future analysis implementations about CGE ML.

Possible modification I can see -

  • [Current] Right now the implemented wrapper class will have to extract all the required data and feed it to the core class. The reason for doing this was to avoid possible future file structure changes that will lead to modifying the core again. Hence, I left the parsing part for analysis to implement.
  • [Improvement] Maybe assume file structure to write general implementations and add an optional argument to accept a parsing function.

Note: Since we will be implementing the analyses so I don't know if this improvement is really needed or not. We could easily structure inputs as required by the Core CGE class.

let me know what your thoughts are and what changes can be made.

@Rashmil-1999 Rashmil-1999 self-assigned this Jan 2, 2024
@Rashmil-1999 Rashmil-1999 linked an issue Jan 2, 2024 that may be closed by this pull request
@Rashmil-1999
Copy link
Contributor Author

I have added the implementation of mmsa cge with a test_mmsacge.py file. all you need to do is to run this to test it. Main thing to look for is improvements in implementation or some modification.

Copy link
Member

@longshuicy longshuicy left a comment

Choose a reason for hiding this comment

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

Could you add an entry in the changelog?

@longshuicy
Copy link
Member

  • left the parsing part for analysis to implement.

Maybe for now you can put the parsing part in a util (instead of on the mmsa class) so in the future other testbed can still reuse them? Just a thought.

@Rashmil-1999
Copy link
Contributor Author

  • left the parsing part for analysis to implement.

Maybe for now you can put the parsing part in a util (instead of on the mmsa class) so in the future other testbed can still reuse them? Just a thought.

yeah, if we can standardize the input files from the econ teams, we can shift it to the core class itself...

@Rashmil-1999 Rashmil-1999 changed the base branch from develop to release-1.18.0 March 26, 2024 15:22
Rashmil-1999 and others added 2 commits April 2, 2024 14:39
* add slc ml cge

* add ml slc cge coefficients

* do some basic modifications

* file names for test

* add changelog

* remove extra files, params, and add dev test client in test

* remove sector-shocks file and unused imports

* update base vals

* bump up precision fo 64 bit floating point in numpy for base vals

* set missing labor group sectors to -1

* remove additional dependency

* change run_analysis to run

* add return statement

* move spec function to the bottom

* some formatting changes

* some formatting changes
@navarroc
Copy link
Member

navarroc commented Apr 3, 2024

To fix the unit tests, I uploaded new files. Can you replace the following?

660d9435ce705a7e547a664e replaces 642f66e5f27db6680103c4ad
660d95bece705a7e547a6654 replaces 642f6230f27db66801038964
660d97bfce705a7e547a6659 replaces 642f6495f27db66801039434

@navarroc
Copy link
Member

navarroc commented Apr 3, 2024

I tested slc cge with this PR and results look good and match results I saved from the SLC CGE branch before it got merged into this branch. Just a few minor things with the copyright. I have not looked through the code exhaustively.

Copy link
Member

@longshuicy longshuicy left a comment

Choose a reason for hiding this comment

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

I ran the slc and connect its output to the playbook utility function (tests/pyincore/utils/test_cgecsvoutputjson.py) with updated region and sector name. All seems to work well. Approving.

@longshuicy longshuicy merged commit 1c08bc0 into release-1.18.0 Apr 4, 2024
6 checks passed
@longshuicy longshuicy deleted the 468-implement-core-cge-ml-class branch April 4, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Core CGE ML class
4 participants