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 matplotlib version issues and a circular import issue #686

Merged
merged 38 commits into from
Jul 22, 2024

Conversation

chansigit
Copy link
Contributor

  • Upgrade matplotlib from 3.5.3 to 3.7.5 (we can actually move forward to 3.9.0 by abandoning python 3.8 )
  • Fix a circular import issue in dym.pp.cell_cycle (this function uses dyn.tl.utils when it is partially imported), I now move imports inside functions group_corr and group_score
  • I also removed python 3.7 tests. Python 3.7 is so old and doesn't support matplotlib3.7.5 and other new version scverse packages.

@chansigit
Copy link
Contributor Author

chansigit commented Jul 4, 2024

@Sichao25 Hi, Sichao, I believe a lot of failed tests were attributed to this issue.

Can we find a better way to store the data? It seems that dropbox banned the non-browser downloading?

One possible solution is to use some professional research data sharing platform like figshare (I've uploaded the data here https://figshare.com/articles/dataset/Dynamo_sample_datasets/25690782).

This works

adata=dyn.sample_data.get_adata(url='https://figshare.com/ndownloader/files/47420257', filename='zebrafish.h5ad')

@Sichao25
Copy link
Collaborator

Sichao25 commented Jul 4, 2024

@Sichao25 Hi, Sichao, I believe a lot of failed tests were attributed to this issue.

Can we find a better way to store the data? It seems that dropbox banned the non-browser downloading?

One possible solution is to use some professional research data sharing platform like figshare (I've uploaded the data here https://figshare.com/articles/dataset/Dynamo_sample_datasets/25690782).

This works

adata=dyn.sample_data.get_adata(url='https://figshare.com/ndownloader/files/47420257', filename='zebrafish.h5ad')

Sounds great. Thank you for detecting this issue.

@chansigit
Copy link
Contributor Author

@Sichao25 Hi, Sichao, I believe a lot of failed tests were attributed to this issue.
Can we find a better way to store the data? It seems that dropbox banned the non-browser downloading?
One possible solution is to use some professional research data sharing platform like figshare (I've uploaded the data here https://figshare.com/articles/dataset/Dynamo_sample_datasets/25690782).
This works

adata=dyn.sample_data.get_adata(url='https://figshare.com/ndownloader/files/47420257', filename='zebrafish.h5ad')

Sounds great. Thank you for detecting this issue.

I've already uploaded all dropbox-based data to figshare and changed the urls in the codes in my own branch. Now trying to pass the github action unit tests.

@chansigit chansigit requested a review from Sichao25 July 5, 2024 08:26
@codecov-commenter
Copy link

codecov-commenter commented Jul 5, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 58.02752% with 183 lines in your changes missing coverage. Please review.

Project coverage is 43.60%. Comparing base (26bc5a5) to head (8d7c134).

Files Patch % Lines
dynamo/plot/utils_dynamics.py 0.00% 17 Missing ⚠️
dynamo/plot/scatters.py 25.00% 15 Missing ⚠️
dynamo/estimation/csc/utils_velocity.py 30.76% 9 Missing ⚠️
dynamo/plot/utils.py 35.71% 9 Missing ⚠️
dynamo/tools/pseudotime.py 50.00% 8 Missing ⚠️
dynamo/plot/sctransform.py 30.00% 7 Missing ⚠️
dynamo/shiny/lap.py 53.33% 7 Missing ⚠️
dynamo/tools/moments.py 0.00% 7 Missing ⚠️
dynamo/estimation/tsc/twostep.py 0.00% 6 Missing ⚠️
dynamo/plot/dynamics.py 25.00% 6 Missing ⚠️
... and 46 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #686   +/-   ##
=======================================
  Coverage   43.59%   43.60%           
=======================================
  Files         161      161           
  Lines       28697    28724   +27     
=======================================
+ Hits        12511    12524   +13     
- Misses      16186    16200   +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Xiaojieqiu
Copy link
Collaborator

@chansigit excellent work! can we also support python 3.10 and 3.11, etc?

Copy link
Collaborator

@Xiaojieqiu Xiaojieqiu left a comment

Choose a reason for hiding this comment

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

I have made a few comments

@@ -15,7 +15,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
python-version: [3.7, 3.8, 3.9]
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we add 3.9, 3.10, 3.11, 3.12?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

3.12 failed due to the removal of distutils package https://stackoverflow.com/questions/77247893/modulenotfounderror-no-module-named-distutils-in-python-3-12

other versions on the way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

3.10 and 3.11 also failed. we need more time to get it pass.
I noticed scanpy is asking a python version <3.10, we may remain in 3.9 temporarily.
https://github.com/scverse/scanpy/blob/db2118e8eb60bbf6287ce1413477480e16b2508b/pyproject.toml#L70

dynamo/configuration.py Outdated Show resolved Hide resolved
dynamo/external/scifate.py Outdated Show resolved Hide resolved
dynamo/plot/topography.py Outdated Show resolved Hide resolved
@Xiaojieqiu
Copy link
Collaborator

@Sichao25 can you please reviewer it and finalize it? I will merge it to the main branch later

Copy link
Collaborator

@Sichao25 Sichao25 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@chansigit chansigit merged commit 117ceb5 into aristoteleo:master Jul 22, 2024
8 checks passed
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