Read model results from files

[Zetison]

This PR enables reading model results from CSV-files (such that the model does not need to be rerun to load them into the GUI). The functionality to write these files are also provided (through the save_results(model::JuMP.Model)-function) which must be used due to the required metadata.yml-file that must be created to run the EMGUI from these files.

[JulStraus]

I won't have too much time within the next week, but I realized two things directly after thinking a bit through what we discussed offline:

1. The approach of using the function match_id seems in my opinion to be one of the major problems with respect to performance. As we use a filter function, we have in practice a for loop every single time we have to receive the value. Would it be beneficial to create a dictionary in the beginning, e.g.,:

   match_id = Dict(repr(n) => n for n ∈ get_nodes(case))

   It can also be extended to include the other parts, although it can lead to more problems as areas and modes are not directly accessible without the extension. That would require a bit of thinking.
2. The problem is even more pronounced for the matching of the time periods. The proposed approach does not work equally nice for the time periods, but it is even more crucial in this case as we have to match a lot of values. One approach would be to rewrite how these are plotted (more complicated, but faster) or alternatively create the required dictionaries based on the time structure.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 87.50000% with 18 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/utils_GUI/GUI_utils.jl	83.15%	16 Missing ⚠️
src/utils_gen/utils.jl	90.90%	2 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Files with missing lines	Coverage Δ
ext/EMGExt/EMGExt.jl	96.07% <100.00%> (+0.95%)	⬆️
src/EnergyModelsGUI.jl	100.00% <ø> (ø)
src/datastructures.jl	90.35% <100.00%> (+0.26%)	⬆️
src/setup_GUI.jl	98.80% <100.00%> (+<0.01%)	⬆️
src/utils_GUI/results_axis_utils.jl	96.15% <100.00%> (+0.39%)	⬆️
src/utils_gen/utils.jl	87.73% <90.90%> (+0.83%)	⬆️
src/utils_GUI/GUI_utils.jl	90.90% <83.15%> (-2.74%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JulStraus]

Comments regarding the actual implementation

I wrote quite a few comments within the code regarding the implementation. It seems to work, but I think there are a few things that can be improved. Specifically, it seems to me that we included some unnecessary hard-coding through if-loops which is against the philosophy. While the hard-coding does not really result in problems yet, it can lead in later stages to problems that are hard to identify. It is at least in my opinion partly unnecessary.

Note

This PR includes two major changes, the implementation of a new feature and a rework of the tests. While I to a certain degree understand why you did both in a single PR, it is still not really a good idea. The actual important changes are drowned within the major changes to the examples. It looks at least to me that it leads to you not writing the tests based on a minimum working example in the test folder, but instead test the new functionality with the examples.
Is this necessary?

Comments regarding topics I oversaw the last time

It is important that we dispatch on functions from EMB or EMG in cases we are using the same name. I found as examples get_elements_vec, get_time_struct, get_products, get_nodes, get_links, get_areas, and get_transmissions. All EMB functions are indirectly dispatched on as you use explicitly import for these functions. It is however better if you do not use import, but instead EMB. as prefix. That makes it in the corresponding method definition clear that you dispatch on an existing method. Otherwise, people always must have the file EnergyModelsGUI open.

This must be fixed in a separate PR as it is not the topic of this PR.

Rework of examples

While I am not opposed to restructuring the examples in such a way (to simplify potential updates to case studies as well as simplify the overview), I am not entirely convinced that this is achieved in this approach. There are a few problems within the current setup introduced through a90997d:

1. The example section in the documentation is not updated. The current version is hence not working. Specifically, you have to add the new file above the include() statement.
2. I would move l. 25-31 in the examples section to the new file generate_examples.jl in the beginning, including a potential instantiation as, e.g., outlined in EnergyModelsGeography. That requires as well minor changes to the file test_examples.jl through introduction of the environment variable. The main thought process here is that we want to use the local version within the tests. In its current design, it does not use the local version, but the latest registered version. As a consequence, the tests are essentially useless.
3. You load some packages in the individual files, while others are loaded in generate_examples.jl. In practice, all packages can be loaded in generate_examples.jl.
4. The @info statements and potential processing of results in the examples could be removed in my opinion. They do not add a lot as the focus is on the GUI, at least the way I see it.
5. The directory in which the variables are saved should be generated as temporary directory via mktempdir.

Tests

In general, I am a bit confused about the meaning of the tests. With the example tests, you test for the examples the functionality of saving and reading from saved files. You only test that within the examples, not at any other point. This makes it honestly significantly more difficult to understand the meaning of the tests as the differing functionality is not tested at consistent places.

I am by no means saying that the other packages have reasonable tests, but I pushed in recent times for more useful tests. As a consequence, I am opposed to adding new tests that are not logically structured.

In addition, you have way too many info prints in the test setup while not showing potential problems. I had a local error due to an empty Array instead of an empty Data[]. I saw it in the summary, but at no other point making it difficult to figure out what the error was. In this case, it is best to suppress all warnings and lower levels.
This is shown in the test run of EnergyModelsRecedingHorizon.jl through the application of the Logging.jl package. Note that one should also activate it again after the tests.

Minor additional comment:

Providing an empty vector for the Data in case7.jl and in the function generate_example_data_geo() is discontinued. It is essentially weird that it was allowed previously. You can just remove the entry as we have constructors.

You can essentially also change all Data[] to ExtensionData[] if you use the latest version of EMB. That would the however require you to update the dependency of EMB to 0.9.1, if this has not happened yet.

[Zetison]

Thanks for your extensive review!

Regaring your note: The examples were put in a function format to be able to avoid opening a GUI when only the output files are required. This results in many lines changes, but it is really just moving content to another file and should therefore not be considered a huge change. I would argue that these type of changes fits this PR.

I changed the code to avoid importing functions from EMB and since most of these imports where introduced in this PR I think it make sense to fix this improvement in its entirety in this PR (no big changes anyways).

Regarding the examples:

1. Fixed
2. Fixed
3. Fixed
4. The @info statements are nice for completeness such that the user can get the full example from, e.g., EMB, and have the GUI on top of that.
5. Fixed

Regarding your info-comment in the tests, I'm not sure how this affects the warnings? Due to the comprehensive tests, it is arguably nice to see that which is currently being processed.

[Zetison]

The information is available once read and interpreted in EMGUI, but for external use and in a standarization context it may make sense to have this. I therefore include it, but convert it to :t once read in the EMGUI. This however, also introduced some hardcoded stuff.

[Zetison]

This is done to limit redundant testing in CI. The other examples does not contribute with much.

[JulStraus]

I went through it and looked through all changes. The unresolved comments are mostly for your information. There is however 1 thing we should stress in both docstring and documentation:

We do with the design not support variables with differing layout than ours for reading in data from csv files. While I understand your current comment, I think that warrants an explicit warning admonition in the documentation (and an issue).

Regarding the @info statements: We have in the current state over 200 lines with @info and @warn statements (l 1147 -1358 in Run the tests (Linux) https://github.com/EnergyModelsX/EnergyModelsGUI.jl/actions/runs/17767322662/job/50493878036).

In the case of any error in any of the test sets, one has to scroll a lot to find what the error is. I added in EMB PR 71 logging in the testset to avoid this info overflow purely when testing. I would really suggest using the same (and set it to Error instead of Warn. The location of a potential error or test fail is anyhow available when utilizing proper testset, so all other information is just coming in addition.

[JulStraus]

I would suggest squash and merge as the majority of the changes are in the examples (1428 removed, 1368 added). Please rewrite the commit message to include the individual contributions. Do not keep the suggested one as there are way too many commits in the PR.

Note that we should in the future not mix things. At least in my opinion, it makes it significantly more difficult to keep an overview.