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

Changed grid to only update one cell if replace value is done #8166

Merged
merged 3 commits into from
Feb 26, 2023

Conversation

ChrisMarsh82
Copy link
Contributor

Updated ReplaceValueInCell to only update the changed cell.

At the moment we still bring back the entire dataframe from R so there is still a delay. This would be a much bigger change and could run into errors. This change should at least half the time taken to refresh

rdstern
rdstern previously approved these changes Feb 23, 2023
Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChrisMarsh82 I tried on a large dataset, where I had noticed the problem before. It had 875,000 rows.
I needed to change temperaures measured such as 305 degrees to 30.5. There were about 11 rows like this. I first tried with a filter, so just the 11 rows were visible. A cell change took about 4 seconds. I bit long, but not impossible.
When I noted the row number and tried on the unfiltered data it took only 2 seconds.

I tried with your stated change and also (for comparison) with the released version. There seemed no change in those times, between the 2 versions.

I had no problem with your new version, so am approving.

Copy link
Contributor

@lloyddewit lloyddewit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChrisMarsh82 Thank you for the changes, just some small comments.
Also, I understand that the purpose of this PR is to increase performance. During his testing, @rdstern found the speed similar to the previous version. Before we increase the code complexity, I think we should demonstrate a speed improvement. Do you have a test case that could show this?

instat/UserControls/DataGrid/Linux/ucrDataViewLinuxGrid.vb Outdated Show resolved Hide resolved
instat/UserControls/DataGrid/ReoGrid/ucrDataViewReoGrid.vb Outdated Show resolved Hide resolved
instat/UserControls/DataGrid/ReoGrid/ucrDataViewReoGrid.vb Outdated Show resolved Hide resolved
instat/ucrDataView.vb Outdated Show resolved Hide resolved
Co-authored-by: lloyddewit <57253949+lloyddewit@users.noreply.github.com>
renamed boolean to have b prefix
changed methods to private
@ChrisMarsh82
Copy link
Contributor Author

@rdstern @lloyddewit The changes I have made only stop the grid being fully loaded from the dataframe. The dataframe is fully refreshed so in some cases it won't appear any quicker.

Where the benefit will come in if you have a large dataset with a lot of rows and where those rows contain lists with lots of values.
For example:
create a dataset with one column with numbers 1 to 1000 (note: create a new dataset is broken at the minute so it will need to contain 3 columns)
add a couple more columns using the calculator "lapply( x1, function(x) {lapply(x, function(i) {choose(i, 0:i)})})"
Now when a value is changed although it still goes back to R to refresh the dataframe it doesn't need to repopulate the list in the grid which was taking a very long time

@lloyddewit lloyddewit changed the title changed grid to only update one cell if replace value is done Changed grid to only update one cell if replace value is done Feb 26, 2023
@lloyddewit lloyddewit merged commit 4066e4d into IDEMSInternational:master Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants