Pandora-IsoMemo / ReSources

Food Reconstruction Using Isotopic Transferred Signals (ReSources)
https://pandora-isomemo.github.io/ReSources/
GNU General Public License v3.0
0 stars 1 forks source link

New bug: loop when deleting a column/row #58

Closed isomemo closed 1 year ago

isomemo commented 2 years ago

For this file: Cluster modelling.zip

I tried to delete the column named c13 by pressing the x to the right of the column's name:

image

This launched a non-ending loop:

image

Loops are shown for all tables but this is strange as tables are supposed to be independent.

arunge commented 2 years ago

@isomemo I think we must discuss, how to solve the issue here.

The loop occurs because the tables are independent but the column and row names cannot be independent.

Row/column names are required to correctly match the data between the target's table and the sources/concentrations tables. The names are needed to select (via the select input) the desired table in sources/concentrations. Therefor the row/column names of list and sub-lists elements are used to label data correctly.

When deleting a row/column in the target's table, it is removed in the target's table but not in sources or concentrations tables.

However, for fixing #34 I added a check if names of target's table and the sources/concentrations tables are matching (if not they are updated automatically -> here tables are dependent). But when one deletes a row this check currently fails and leads to the endless loop.

The only chance I see to fix this is to automatically delete all respective list entries in sources/concentrations if one deletes a target's row or column. Then the check for matching names will work again.

However, this will make the tables depend on each other. And I see no other way to solve this. Please, let us discuss how to continue here.

isomemo commented 1 year ago

@arunge we can discuss this today! However, the best way seemt to be to make the columns and row names independent accross tables. Leaving the user to ensure that these match. Once the user runs the model a check is made if names are matching and if not an error message is generated and the model is not run.

arunge commented 1 year ago

As mentioned here sources and concentration tables must be dependent on the table names of the targets table. If I do not update the list names of sources and concentration tables if one changes the column / row names of the target's table, the tables will again be emptied when column names are changed. (We solved that bug). Here you see were column- (2) /rowname (1) changes must be updated:

when we change the names in the target's table:

image

However, deleting a column is still an open issue which I did not fix yet.

Yes, let us discuss this issue in the call.

arunge commented 1 year ago

As discussed in our last meeting, we keep the current structure and I fix the issue with deleting a column.

arunge commented 1 year ago

I could fix the bug with deleting rows/columns with ReSources 22.12.1 on the beta version. When the checks have finished, the new version will be available.

In order to fix this bug, I had to restructure the logic for updating table columns and rows. I added several new tests that helped me to understand the issue while developing a solution. In ReSources, there are many dependencies between tables and model inputs (that is why it takes so long to load the app). I hope, there is no issue left and that I could catch all different cases, such as baseline model / no baseline model or use / not use components.

@isomemo Please test carefully several scenarios with maybe different data and let me know if there is still an open issue. Thanks for your patience!

isomemo commented 1 year ago

@arunge this will take some time to test. I am also preparing instructions for Marcus to fix other model-related things.

arunge commented 1 year ago

@isomemo Because of several bug fixes that were still only on beta but not on the main, and in order to reduce confusion what is on which version, I decided to push the current beta version to the main app. (In case this causes problems: it is always possible to revert a commit with git.)

You already opened a new issue because there are still some issues that we must discuss. I think, I can close here.

isomemo commented 1 year ago

@arunge Ok!