Resolve "GDX improter unable to read multidimensional symbols" - [merged] #1175

Closed spine-o-bot closed 3 years ago

In GitLab by @ererkka on Jun 3, 2020, 16:18

Merges 730-gdx-improter-unable-to-read-multidimensional-symbols -> dev

Closes #730

In GitLab by @ererkka on Jun 3, 2020, 16:47

added 1 commit

d3d9cf9c - Fall back to a general header if no domain available from gdx file

Compare with previous version

In GitLab by @ererkka on Jun 3, 2020, 17:09

@soininen, could you check that this is ok? All tests pass. This is how it looks:

In GitLab by @ererkka on Jun 3, 2020, 17:10

unmarked as a Work In Progress

In GitLab by @ererkka on Jun 4, 2020, 15:33

assigned to @soininen

In GitLab by @ererkka on Jun 4, 2020, 15:33

marked as a Work In Progress

In GitLab by @ererkka on Jun 4, 2020, 15:33

This might have broken the importer. I get:

Traceback (most recent call last):
  File "c:\data\python\spinetoolbox\spinetoolbox\project_items\importer\importer_program.py", line 161, in 
    run(*json.loads(input()))
  File "c:\data\python\spinetoolbox\spinetoolbox\project_items\importer\importer_program.py", line 93, in run
    table_mappings, table_options, table_types, table_row_types, max_rows=-1
  File "c:\data\python\spinetoolbox\spinetoolbox\spine_io\io_api.py", line 104, in get_mapped_data
    data, t_errors = read_with_mapping(data, mapping, num_cols, header, types, row_types)
  File "C:\Users\ERERKKA\AppData\Local\Continuum\miniconda3\envs\spinetoolbox\lib\site-packages\spinedb_api\json_mapping.py", line 1587, in read_with_mapping
    r = m.create_mapping_readers(num_cols, pivoted_data, data_header)
  File "C:\Users\ERERKKA\AppData\Local\Continuum\miniconda3\envs\spinetoolbox\lib\site-packages\spinedb_api\json_mapping.py", line 1314, in create_mapping_readers
    component_readers = self.create_getter_list(pivoted_columns, pivoted_data, data_header)
  File "C:\Users\ERERKKA\AppData\Local\Continuum\miniconda3\envs\spinetoolbox\lib\site-packages\spinedb_api\json_mapping.py", line 1303, in create_getter_list
    *create_getter_list(self.objects, pivoted_columns, pivoted_data, data_header),
  File "C:\Users\ERERKKA\AppData\Local\Continuum\miniconda3\envs\spinetoolbox\lib\site-packages\spinedb_api\json_mapping.py", line 1691, in create_getter_list
    o, num, reads = map_.create_getter_function(pivoted_columns, pivoted_data, data_header)
  File "C:\Users\ERERKKA\AppData\Local\Continuum\miniconda3\envs\spinetoolbox\lib\site-packages\spinedb_api\json_mapping.py", line 256, in create_getter_function
    ref = data_header.index(ref)
ValueError: '0' is not in list

In GitLab by @ererkka on Jun 4, 2020, 16:58

Seems that this happens with empty symbols.

In GitLab by @ererkka on Jun 4, 2020, 17:24

This problem is also in dev, I created #733.

In GitLab by @ererkka on Jun 4, 2020, 17:24

resolved all threads

In GitLab by @soininen on Jun 8, 2020, 10:22

added 1 commit

fa2bb122 - Fix sets using universal and named dimensions

Compare with previous version

In GitLab by @soininen on Jun 8, 2020, 10:30

The unit tests did not pass, in fact. When running any test that uses the gdx2py package one has to make sure the test is not skipped by a missing or broken GAMS installation. Note, that all these tests are skipped on Travis.

In any case, I took the liberty to improve the solution to work also with multidimensional symbols that have both named and placeholder dimensions, such as n(i,*,j).

One thing I want to discuss is the "dim0", "dim1" etc. placeholder name we have chosen to use instead of * here. Is this normal parlance in GAMS community? Why "dimX"? I would either use just * as before or "Dimension X" where X is a 1-base index (not zero-base). But I don't know what would make most sense for actual GAMS users.

In GitLab by @soininen on Jun 8, 2020, 11:03

unmarked as a Work In Progress

In GitLab by @ererkka on Jun 8, 2020, 11:11

Your concern is right, dimX is not GAMS parlance.

I wanted to separate the GAMS universal set '*' from what the user sees in the Toolbox as they become column headers. In my opinion, it might be confusing if multiple columns have the same header '*'. The symbols might also have proper domains defined in the final model, but the imported GDX file is lacking the domain info. In this case I’d use something that can not be confused with the actual domains nor with the universal set. dimX could also be 1-based.

In GitLab by @ererkka on Jun 8, 2020, 11:17

I’m getting the ‘A row contains too many columns of data.’ error again with the latest commmit :( A three-dim symbol has only header ['dim0'].

In GitLab by @ererkka on Jun 8, 2020, 11:21

It’s from this:

domains = symbol.domain if symbol.domain is not None else [None]

if there’s no domain, only one-length list [None] is stored, and that then becomes ['dim0'] on the following line

header = [domain if domain is not None else f"dim{i}" for i, domain in enumerate(domains)]

In GitLab by @soininen on Jun 8, 2020, 14:55

added 1 commit

605a08bf - Fix universal set dimensions gdx connector

Compare with previous version

In GitLab by @soininen on Jun 8, 2020, 14:56

Nice catch! I should have written a unit test for this specific case as the very first thing I did on this branch. Hopefully it is fixed now.

In GitLab by @soininen on Jun 8, 2020, 15:16

Personally I don't see a problem with multiple *. I think it makes sense to use * there because in GAMS one would write k(i,j,*,*) and get the following column headers for the k table in Import editor:

| i | j | | |

Wouldn't the above be more familiar than

| i | j | dim0 | dim1 |

for a GAMS user?

In any case, I don't think this is a big deal and can be changed later if needed. Lets leave the headers as dim0, dim1 etc. for now.

In GitLab by @soininen on Jun 8, 2020, 16:09

added 1 commit

07d21d8c - Cleanup, black formatting. Re #730

Compare with previous version

In GitLab by @ererkka on Jun 8, 2020, 16:38

merged

In GitLab by @ererkka on Jun 8, 2020, 16:38

mentioned in commit 7d797893c3422a121df5e879d330a5e0e48901ef

spine-tools / Spine-Toolbox

Resolve "GDX improter unable to read multidimensional symbols" - [merged] #1175