Integration of LPdiag tool

[glatterf42]

As discussed on Slack, this branch contains the files of the MCA tool that @marek-iiasa created. Unfortunately, I couldn't (without significantly more work) keep the commit history and move the files to their new location. Please let me know if that should still be done.
Please find the original repo at https://github.com/marek-iiasa/MCA/tree/master. Note that e.g. the reading material has not yet been included here.

How to review

• Please check out this branch and try to work with this tool

PR checklist

• Continuous integration checks all ✅ (run black on python files)
• Add or expand tests; coverage checks both ✅
• Add, expand, or update documentation.
• Update release notes.
• Please adapt the README files to reflect the new environment
• Please think about closer integration options other than simply having the files copied here

[codecov]

Codecov Report

Merging #704 (2a6c423) into main (d36f676) will increase coverage by 0.4%.
The diff coverage is 98.4%.

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #704     +/-   ##
=======================================
+ Coverage   94.8%   95.2%   +0.4%     
=======================================
  Files         43      46      +3     
  Lines       3797    4312    +515     
=======================================
+ Hits        3600    4107    +507     
- Misses       197     205      +8     

Files	Coverage Δ
message_ix/cli.py	92.9% <100.0%> (+0.1%)	⬆️
message_ix/tests/tools/test_lp_diag.py	100.0% <100.0%> (ø)
message_ix/tools/lp_diag/__init__.py	98.9% <98.9%> (ø)
message_ix/tools/lp_diag/cli.py	91.8% <91.8%> (ø)

[marek-iiasa]

Location: I propose to drop the MCA level from the tree, i.e., to move LPdiag dir directly below the tools. I would keep the LPdiag name unless someone will propose a better name.

Alternative names for possible consideration: LPcheck, LPinfo.

---

This was solved. The tool is called lp_diag, and it is moved to the right place: .../ tools/lp_diag

[marek-iiasa]

Unfortunately, I cannot contribute to the PR review:

• The code works on iMac (ventura) and MacBook in pyCharm with py 3.10. I have no idea what/why errors occur on other OSs
• I will extend the functionality after having suggestions from colleagues on what is desired.

I will update the readme.txt files, and later write some documentation. However, I assume that pushing any changes should wait until the initial PR will be accepted and the files will be in a better place than pr/704

[marek-iiasa]

Unfortunately, I couldn't (without significantly more work) keep the commit history and move the files to their new location. Please let me know if that should still be done.

Thanks for moving the stuff. Retrieving the commit history is certainly not worth additional work.

[CLAassistant]

All committers have signed the CLA.

[glatterf42]

Thanks for the updates :)
A few things to note:

• You added some .gitignore files. In general, these shared exclusion files should only contain exclusion patterns that other people might also reasonably have. For example, the .swp you added to the main .gitignore seems like the exclusion of a certain file type that others should not commit, either. But of_*, as you specified in message_ix/tools/lp_diag/data/mps/.gitignore, seems like these are only files that concern yourself locally (and same for all directories listed in message_ix/tools/lp_diag/.gitignore). While we don't have a strict rule for this, I think it is strongly preferred to exclude these files only locally. You can achieve this by adding them to the file .git/info/exclude (see the docs here).
• message_ix/tools/lp_diag/Readme.txt includes some mention of a directory that will not be accessible to the public, so there is not much gained by mentioning it. Could you please rework that section? For sharing large files, the git large file storage might be an option (but that could also clutter our repositories) or you might be interested in checking out zenodo.
• The main format we use for documentation is restructured text (rst). This grants additional features like links and more formatting options than txt. For the README files that are only intended to show information on GitHub, txt might be fine, but our landing page is an md file, which also has more options than txt. For the documentation itself, rst is necessary since the docs are built automatically using sphinx.
• Also regarding the documentation, is there any specific reason not to include a short description of how the tool currently works? If we merge the PR now, we will release this tool without people knowing how to use it or what to use it for; it will take another PR to update the documentation and things like scope creep are real: you start a PR for working on the docs, but you keep finding little things to include and so the PR doesn't get merged for a long time. Please include some documentation, however preliminary, here.
• Please note that there is no need to repeat yourself: compare the add_year tool. The doc/tools/add_year.rst is simply a reference to the rst file that had already been written. You could do the same here.
• About adding to the release notes, I can add a note that this tool is now included with message_ix. Please just let me know how to describe it properly (ideally in one line), something like: Introduce the LPdiag tool to ...

[marek-iiasa]

Thanks for the updates :) A few things to note:

Thanks for comments and suggestions. Please see my reaction to each issue below:

• You added some .gitignore files. In general, these shared exclusion files should only contain exclusion patterns that other people might also reasonably have. For example, the .swp you added to the main .gitignore seems like the exclusion of a certain file type that others should not commit, either. But of_*, as you specified in message_ix/tools/lp_diag/data/mps/.gitignore, seems like these are only files that concern yourself locally (and same for all directories listed in message_ix/tools/lp_diag/.gitignore). While we don't have a strict rule for this, I think it is strongly preferred to exclude these files only locally. You can achieve this by adding them to the file .git/info/exclude (see the docs here).

I will review and implement exclusions of the files that indeed should be only local. BTW: I am indeed convinced that nobody would like to commit the *.swp files.

• message_ix/tools/lp_diag/Readme.txt includes some mention of a directory that will not be accessible to the public, so there is not much gained by mentioning it. Could you please rework that section? For sharing large files, the git large file storage might be an option (but that could also clutter our repositories) or you might be interested in checking out zenodo.

I will review and correct this.

• The main format we use for documentation is restructured text (rst). This grants additional features like links and more formatting options than txt. For the README files that are only intended to show information on GitHub, txt might be fine, but our landing page is an md file, which also has more options than txt. For the documentation itself, rst is necessary since the docs are built automatically using sphinx.

I have already explored sphinx a little bit, and will write the *rst version after we will clarify several small issues pointed out below.

• Also regarding the documentation, is there any specific reason not to include a short description of how the tool currently works? If we merge the PR now, we will release this tool without people knowing how to use it or what to use it for; it will take another PR to update the documentation and things like scope creep are real: you start a PR for working on the docs, but you keep finding little things to include and so the PR doesn't get merged for a long time. Please include some documentation, however preliminary, here.

Indeed, we should wait with merging until the documentation will be in a good enough shape. Regarding the usage: I want to discuss this issue with at least Oliver as I have only the developer perspective. We also need to add the a hint on generating the MPS-format file from a message_ix generated model instance; also here I need help of Oliver.

• Please note that there is no need to repeat yourself: compare the add_year tool. The doc/tools/add_year.rst is simply a reference to the rst file that had already been written. You could do the same here.

I will keep this in mind when writing *rst. BTW: please let me know, how to locally generate (using the message_ix style/config) the resulting *html version?

• About adding to the release notes, I can add a note that this tool is now included with message_ix. Please just let me know how to describe it properly (ideally in one line), something like: Introduce the LPdiag tool to ...

I would consider:
Introduce the LPdiag tool to support analysis of numerical characteristics of the generated optimization problem.

Also here Oliver, and possibly other colleagues, may have better ideas.

[glatterf42]

Thanks, Marek, for your detailed answer. Just pinging @OFR-IIASA here to put this PR on this radar.

For building the sphinx docs locally, we have this guide. Please let me know if this is not complete or needs updates.

[glatterf42]

Dear @marek-iiasa and @measrainsey, please take a look at this PR again. While adding some tests today, I restructured some aspects of the tool, so please make sure it still behaves as intended. Among others, I renamed some functions (e.g., lpdiag.stat() to lpdiag.print_statistics()) so that the names are more intuitive to use. I've also converted the general raise Exceptions and some prints to raising specific error messages, so the code might fail on more occasions than before.
Apart from that, the code here is up-to-date with main and has enough test coverage now to be merged. So we can do that if you have no more additions to the docs or similar. Of course, there is probably also a more elegant way to test these error cases, feel free to update the existing tests as you like.
Please let me know what you think :)

[glatterf42]

Please note: you can click on 'Show all checks', find the one called 'docs/readthedocs.com:iiasa-energy-program-message-ix' and click on 'Details' to see the docs as built from the files in this PR :)
However, maybe the current build was triggered too soon after the last successful one, the changes from 'Update documentation' don't seem to be included yet.

[khaeru]

Some initial clean-ups:

• The "README" file should just be moved to doc/tools/lp_diag.rst, not .. include::-ed. This approach is fragile.

• The contents of the other README files should be included in the docs.

• The .gitignore files should be integrated with the top-level .gitignore.

• message_ix/tools/lp_diag/test_mps/… should be message_ix/tests/data/lp_diag/…

  • Flatten all the files to a single directory. The ones in the …/error/… subdirectory can be renamed, for instance, error-bounds-too-short.mps.
  • Add the .mps extension.

• There are two modules introduced:

  • message_ix.tools.lp_diag.lp_diag
  • message_ix.tools.lp_diag.lpdiag

  The basic principle is that (sub)module names should remind us what the (sub)modules contain. To achieve that, we should avoid (1) duplicate names in the hierarchy (What does .lp_diag.lp_diag contain that is not in .lp_diag itself?), and (2) names that differ only by an underscore.

The strategy of placing the I/O files within the code directory is also not compatible with some kinds of Python installations, and we avoid this almost always. The code should default to using the directory in which the user invokes the CLI (the CWD, or current working directory). I can investigate (separately) adding a "local data" configuration setting that can point to another path, but again this path should be outside the code tree.

[glatterf42]

I've updated the PR to comply with your suggestions, thanks @khaeru. For the last point, though, I think we will need your input, @marek-iiasa.

One note about .gitignore files: there are two of them in tutorial and tutorial/westeros, so two more that are not on the top level. Should these also be merged with the top level one?

[glatterf42]

Thanks for your contribution here, @ywpratama. Unfortunately, your commit broke several tests ... as does mine. I thought this might be fixed by rebasing on top of main, but apparently, that's not the case. I will have to investigate. Sorry for overwriting your commit.

[khaeru]

In the interest of time, I'll step in here to make some of these changes myself:

• Choose module names, per the last review.
• Conform to the code style:
  • Reword some commit messages.
  • Fix docstrings.
• Use a fixture for the test data path.
• Integrate the CLI.

[khaeru]

Okay, I've made the changes mentioned above, so LGTM once the CI checks pass.

Some other things which could be handled in a follow-up PR:

• Copy-edit the docs page. There are several small enhancements that could be made, like using Sphinx roles instead of double-backtick in many places, and improving the clarity of text about the locations of files. In particular, the command no longer needs to be run in any particular directory as long as the user gives valid values for --wdir and --mps.
• Define, type, and comment the class attributes of LPdiag outside of the __init__() method so they appear in the docs; use that method only for setting defaults.