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

Empty values and column-data-storage refactor #5367

Merged
merged 18 commits into from
Apr 2, 2024

Conversation

JorisGoosen
Copy link
Contributor

@JorisGoosen JorisGoosen commented Jan 3, 2024

Empty values should be stored in the data and per column.

To do this properly it required a lot of changes to the underlying code.
Ill make sure the documentation for the classes at least resembles what is going now ;)

Possible problems:
[X] importing from 0.17.3 and earlier (converting from emptyValuesMap in metadata.json)
Ignore: importing from 0.18.2 (broken emptyvalues (i might just ignore it))
[X] importing from 0.18.3 (breaking up emptyvalues into per workspace and column)
[X]] also making sure that we can still import scalar columns with a non-scalar value in them that is not convertible to a double (like in the testcsv tsv ods etc)
Should probably be a separate PR: [] Add locale support (should still be done but testing will take time anyway)
[] importing a nominal/ordinal now as scalar means getting the ints we wrote to the _ints column. We should probably try to use the "value" shown in the label-editor instead! :o This will help with jasp-stats/jasp-issues#2530 or jasp-stats/jasp-issues#2325 ) (But most of that will probably be separate)

@JorisGoosen JorisGoosen marked this pull request as draft January 3, 2024 15:59
@JorisGoosen JorisGoosen force-pushed the development branch 2 times, most recently from 51a94ae to b41342d Compare January 10, 2024 16:28
@JorisGoosen JorisGoosen force-pushed the emptyValuesRefactor branch 2 times, most recently from ba5831e to ddd1efa Compare January 30, 2024 12:35
@JorisGoosen JorisGoosen changed the title Empty values refactor Empty values and column-data-storage refactor Feb 13, 2024
@JorisGoosen
Copy link
Contributor Author

image

@JorisGoosen
Copy link
Contributor Author

I started a bunch of builds under emptyValuesRefactor in case anybode feels like testing it out.

I think this is probably ready now?

@JorisGoosen JorisGoosen self-assigned this Mar 7, 2024
@JorisGoosen JorisGoosen marked this pull request as ready for review March 7, 2024 20:31
@JorisGoosen
Copy link
Contributor Author

Its maybe not completely done, but close enough that you guys could start reviewing it.
And Ill also ask people to test it once the nightlies are build because im sure there are some things ive missed...

@JorisGoosen
Copy link
Contributor Author

macos arm
macos x86

Aah, windows doesnt work it seems...

@shun2wang
Copy link
Contributor

the build test failed maybe also failing on windows.

@RensDofferhoff
Copy link
Contributor

  • Press new data
  • add any string in an empty column
  • No Type is detected
  • Use this column in descriptives and the engine crashes

@JorisGoosen JorisGoosen force-pushed the emptyValuesRefactor branch 2 times, most recently from a88bea3 to 6beffc4 Compare March 12, 2024 13:13
@JorisGoosen
Copy link
Contributor Author

JorisGoosen commented Mar 12, 2024

  • Press new data

    • add any string in an empty column

    • No Type is detected

    • Use this column in descriptives and the engine crashes

Oops!

Edit: shouldve set columntype with the default values...
Nightlies are building with this fix for anyone curious

@JorisGoosen
Copy link
Contributor Author

Aah moving labels also doesnt work anymore for columns where all values are convertible to double

@boutinb
Copy link
Contributor

boutinb commented Mar 12, 2024

Some issues:

  • The empty values are empty for a column, though the workspace has some empty values
  • In Edit mode, if I change a value, and press the tab key, the change is lost
  • With a nominal column, if I set a new value, it does not appear in the Label Editor.
  • I have tried to add a computed column with just contNormal + 1: the engine crashes

Nice feature to see the labels for a scale column (if they are not the same as the value). It could be also nice to see the value of nominal or ordinal column, if the label and the value is not the same.

@JorisGoosen JorisGoosen reopened this Mar 13, 2024
boutinb and others added 9 commits March 28, 2024 16:22
If a cell has a custom empty value, in a Edit mode when clicking on this cell, this removes the custom empty value.
For example, add 1 as empty value for the facFive column. Click on a cell that has 1 as value (it is displayed in grey), and go to another cell: the value 1 is removed.
also makes sure to show the variableswindow and thus code-editor on insertion only for computed columns.
This to allow for rapid addition of computed columns

Also refactors a bit of the menu code in DataTableView
draghand cursor for cells ;)
use same double formatting for label
undo for label originalvalue should remember label and set it back
fix Process data library file so it will actually load
set some code font in a few places
dont set columntype to unknown when running setDefaultValues
add a codepath for forcing all columns to the same columntype for computed columns
@shun2wang
Copy link
Contributor

FileOperation menu has some overlap with the , especially for language-specific strings lengths.

image

@JorisGoosen
Copy link
Contributor Author

Ive also added a new feature to computed columns, see: https://github.com/jasp-stats/INTERNAL-jasp/issues/2487

…tely empty scalar cells

also malkes sure the keep and convert input columntypes button always actually shows the correct value
@boutinb
Copy link
Contributor

boutinb commented Apr 2, 2024

When reading a JASP file from 0.18.0 where for example 0 was set as an empty value, JASP displays the values that had 0 as empty values in the data view, but does not display the original value (0) in grey, and it does not set in the workspace that 0 is an empty value.

Sorry bad manipulation. It works: the 0 is set as empty value in the workspace. Only minor thing is that the if 0 is unset, the cells are not set back to 0: the original value (0) is lost.

@boutinb
Copy link
Contributor

boutinb commented Apr 2, 2024

Sorry bad manipulation. It works: the 0 is set as empty value in the workspace. Only minor thing is that the if 0 is unset, the cells are not set back to 0: the original value (0) is lost.

I have built it with the last commits, and now if I load a JASP file from 0.18.0, the workspace empty values are empty!

@JorisGoosen JorisGoosen merged commit 2d8e7f6 into jasp-stats:development Apr 2, 2024
1 check passed
@JorisGoosen JorisGoosen deleted the emptyValuesRefactor branch April 2, 2024 09:52
@JorisGoosen
Copy link
Contributor Author

I only made an effort to support pre-0.18.0 versions of JASP and specifically 0.18.3.
This because there were some quite terrible bugs in those earlier 0.18.* versions and because we released them in such short order I think we can safely ignore them.

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.

4 participants