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

Fix Clustergram reordering of row and column indices. #458

Merged
merged 11 commits into from
Jan 7, 2020

Conversation

mkcor
Copy link
Contributor

@mkcor mkcor commented Jan 4, 2020

Closes #432.

About

This is a hot fix for the Clustergram component, which used to shuffle rows and/or columns (depending on whether clustering would be along rows, columns, or both) after they were reordered (as expected) from clustering (based on scipy's hierarchy submodule).

All the action is taking place at commit 611a1b3 (the rest is minor).

I included a unit test (bda8f08) using the example from the bug report (#432).

Description of changes

Before merging

@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-458 January 4, 2020 21:17 Inactive
@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-458 January 4, 2020 21:23 Inactive
@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-458 January 4, 2020 21:24 Inactive
@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-458 January 5, 2020 13:26 Inactive
@mkcor
Copy link
Contributor Author

mkcor commented Jan 6, 2020

I wonder why test_dbsv004_coverage_clicked is failing for Python 3.7... 🤔

Copy link
Contributor

@shammamah-zz shammamah-zz left a comment

Choose a reason for hiding this comment

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

💃 Once we figure out what is causing that test to fail. Have you done a fresh npm ci/npm run build/pip install -e .?

@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-458 January 6, 2020 18:36 Inactive
@mkcor
Copy link
Contributor Author

mkcor commented Jan 6, 2020

dancer Once we figure out what is causing that test to fail. Have you done a fresh npm ci/npm run build/pip install -e .?

@shammamah must have been that, thanks ;)

@shammamah-zz shammamah-zz self-requested a review January 6, 2020 18:49
@shammamah-zz
Copy link
Contributor

@mkcor Please add a CHANGELOG entry and bump the version! I can publish to npm and PyPI.

@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-458 January 6, 2020 19:31 Inactive
@mkcor
Copy link
Contributor Author

mkcor commented Jan 6, 2020

Aww what's wrong with file dash_bio/async~moleculeviewer3.js? :(

@shammamah-zz
Copy link
Contributor

You might have to do a full clean (rm -rf node_modules) and npm ci. :/

@mkcor
Copy link
Contributor Author

mkcor commented Jan 6, 2020

When I clean up first, I get the same postbuild error ("postbuild": "es-check es5 dash_bio/*.js") upon running npm run build (after rm -rf node_modules and npm ci)...

@mkcor
Copy link
Contributor Author

mkcor commented Jan 6, 2020

I'll try upating eslint.

@mkcor
Copy link
Contributor Author

mkcor commented Jan 6, 2020

Oh dear, I went through https://flaviocopes.com/update-npm-dependencies/ but still getting this eslint postbuild error.

@shammamah-zz
Copy link
Contributor

@mkcor Looking into it now!

@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-458 January 6, 2020 21:53 Inactive
@shammamah-zz
Copy link
Contributor

@mkcor We don't want to update our npm packages here (that's why I suggested npm ci). Would you mind reverting those updates?

@mkcor
Copy link
Contributor Author

mkcor commented Jan 6, 2020

Duh, I've been trying to run es-check es5 dash_bio/*.js while reading that I should upgrade es5 to es6! Pff, I shouldn't work when feeling sickish. Expect a new commit soon!

@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-458 January 6, 2020 21:59 Inactive
@mkcor
Copy link
Contributor Author

mkcor commented Jan 6, 2020

@mkcor We don't want to update our npm packages here (that's why I suggested npm ci). Would you mind reverting those updates?

Sure. We need to update eslint though (see AtomLinter/linter-eslint#592 and apparently that's the spirit of our .eslintrc file). Is it okay if I revert all the package updates except for that of eslint?

Broader question: Why don't we want to update packages? Originally we set up #377 for this purpose.

@shammamah-zz
Copy link
Contributor

Sure. We need to update eslint though (see AtomLinter/linter-eslint#592 and apparently that's the spirit of our .eslintrc file). Is it okay if I revert all the package updates except for that of eslint?

That sounds fine.

Broader question: Why don't we want to update packages? Originally we set up #377 for this purpose.

We don't want to update npm packages (that don't need to be updated) in this PR, since it only modifies one of the Python components.

@shammamah-zz
Copy link
Contributor

Also, doing a clean install/build from master doesn't give the same error, so since you've only modified the Python parts, there logically shouldn't be any build issues here at all. Nothing has changed on the JS side.

This commit looks like it was the culprit: d2faa89

It seems to have changed the versions of a bunch of packages. Please check out the versions of package.json and package-lock.json that are on master, then rm -rf node_modules/npm ci/npm install.

@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-458 January 6, 2020 22:31 Inactive
@mkcor
Copy link
Contributor Author

mkcor commented Jan 7, 2020

Also, doing a clean install/build from master doesn't give the same error, so since you've only modified the Python parts, there logically shouldn't be any build issues here at all. Nothing has changed on the JS side.

Right, my install/build at d2faa89 must have been messy.

This commit looks like it was the culprit: d2faa89

Thanks for looking into it! I have rebased to remove this commit and all subsequent commits. Then:

  • rm -rf node_modules/
  • edited package.json to bump Dash Bio's version to 0.4.6
  • npm ci
  • npm install
  • committed (b09ff8f)

I updated the date to today's date in the changelog. Then:

  • npm run build
  • committed (e43023f)

Please review the visual change (Percy), thanks!

Copy link
Contributor

@shammamah-zz shammamah-zz left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this! 💃

@mkcor mkcor merged commit 1ba0536 into master Jan 7, 2020
@mkcor mkcor deleted the fix-clustergram-reordering branch January 7, 2020 16:46
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.

Clustergram not reordering heat-map rows
2 participants