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

Adds a public API to convert and convert_results methods #180

Merged
merged 15 commits into from
Aug 18, 2023

Conversation

willu47
Copy link
Member

@willu47 willu47 commented Jun 20, 2023

In previous versions of otoole, the internal read and write strategies were exposed to the user. This was not ideal as it meant that the user had to know the internal structure of the model. This commit adds a public API to convert, read, write and convert_results methods. These methods are used to read, write and convert between different file formats for input data, and convert from solution files to a folder of csv files for results.

Description

This PR also:

  • adds tests for the convert, read, write and convert_results methods
  • adds a new module to the otoole package called convert.py
  • adds a test fixture for the super_simple model
  • fixes typing errors
  • bumps Amply to version 0.1.6

Issue Ticket Number

#181

Documentation

Added a documentation page "Python API" with a short outline of the new functions, and links to the module documentation. Docstrings are filled out in detail.

In previous versions of otoole, the internal read and write strategies
were exposed to the user. This was not ideal as it meant that the user
had to know the internal structure of the model. This commit adds a
public API to convert and convert_results methods. These methods
are used to convert between different file formats.

This commit also:

- adds tests for the convert and convert_results methods
- adds a new module to the otoole package called convert.py
- adds a test fixture for the super_simple model
- fixes typing errors
@willu47 willu47 requested a review from trevorb1 June 20, 2023 17:39
@trevorb1
Copy link
Member

This looks so cool, @willu47!! My schedule is pretty tight for the rest of this week, but I should have time early next week to review this and give feedback!

@trevorb1
Copy link
Member

trevorb1 commented Aug 10, 2023

Hi @willu47! I am so sorry this took me nearly 2 months to review.

This is a really excellent addition! I ran through the examples you posted in the Python API documentation and was able to successfully complete them all! I have three notes on the PR:

  1. The API functions have positional arguments ordered so the configuration file is passed first. The CLI functions have the configuration file as the last positional argument. Would it make sense to coordinate these, so the config file is always either the first or last positional argument?

  2. The current API functions include convert(...), convert_results(...), read(...), and write(...). Would it make sense to also include a function to directly read in solver results (such as a read_results(...) function that returns the dictionary of dataframes)?

  3. The documentation page of Python API is great! I was able to very easily follow your examples. I am just wondering about how to coordinate these examples with the CLI examples on the Examples page. Do you think it would make sense to try and combine these into one page? I am thinking that may be a little overwhelming though. Or maybe we try to mirror the examples on the CLI page to the Python API page. Or maybe I am just overthinking this and doing as you have already done by linking the module reference is good!

Wherever we land on these points, I am happy to help address them if needed! :)

@willu47
Copy link
Member Author

willu47 commented Aug 17, 2023

Hi @trevorb1 - see the latest commits for adding a read_results function and refactoring the CLI to now use the new Python interface, resulting in a cleaner separation.

I have also "fixed" the awkward flags in the otoole results CLI, replacing them with two positional arguments for the input data format and path. This means that input data is now required for result processing, but that all input formats will be read in through the otoole's own read interface (and it will be easier to support new formats as they are added).

I haven't moved the config arguments around, as I feel that CLI and Python interfaces will be used separately, and the documentation is such that it is easy to follow e.g. docstrings for Python interface, and the integrated help dialog for the CLI.

@willu47 willu47 mentioned this pull request Aug 17, 2023
src/otoole/input.py Outdated Show resolved Hide resolved
docs/functionality.rst Outdated Show resolved Hide resolved
docs/functionality.rst Outdated Show resolved Hide resolved
@willu47 willu47 merged commit b24a24b into OSeMOSYS:master Aug 18, 2023
3 checks passed
@willu47 willu47 deleted the python_interface branch August 18, 2023 06:51
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.

2 participants