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 auto to best #1043

Merged
merged 1 commit into from
Feb 19, 2021
Merged

changed auto to best #1043

merged 1 commit into from
Feb 19, 2021

Conversation

LeilyR
Copy link
Contributor

@LeilyR LeilyR commented Feb 18, 2021

Welcome to deepTools GitHub repository! Please check the following regarding
your pull request :

@dpryan79
Copy link
Collaborator

The galaxy tests are expected to fail then, since some of them will compare the images. Can you see if you can determine when matplotlib added "best" so we can update the matplotlib version restriction?

@LeilyR
Copy link
Contributor Author

LeilyR commented Feb 19, 2021

sure, i will have a look

@LeilyR
Copy link
Contributor Author

LeilyR commented Feb 19, 2021

The best option seems to be already availble since version2, this was taken care of for the cli , the default was already set to best, but in the init was auto which i suppose will through an error only if one uses the api. Planemo test seems to be failed at deeptools_bam_pe_fragmentsize where comparing some tables, some little differences between values (241.20000000000002 instead of 241.2). I thought it could be related to the server the tests are run on it, so I tried to re-run the tests but it did not help. If you do not mind I ginore the error for now and merge this little bug fix to develop desipte the error and we see if it comes up we merging to master again and fix it then.
In general I wonder if we should remove this type of tests from planemo, since it really seems that it is not capable of handelign them. Do you have any idea for that?

@dpryan79
Copy link
Collaborator

It's a double-edged sword. The benefit to keeping them in is we catch more subtle issues with plotting. So I would vote to keep them in, but wouldn't be terribly disappointed if you said, "Gah! I've had enough of these false-positive failures. Let's remove them!" :)

@LeilyR
Copy link
Contributor Author

LeilyR commented Feb 19, 2021

Lets keep them then :) I can always ignore these errors :P You are right, it is better to see them and ignore them than not knowing what went wrong. Shall i merge this to develop now?

@dpryan79
Copy link
Collaborator

Sure, 👍 on merging from my side :)

@LeilyR LeilyR merged commit 2c4febd into develop Feb 19, 2021
@LeilyR LeilyR deleted the fix_1042 branch March 15, 2021 16:26
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.

2 participants