Conversation
start with PEP compliance, add f-strings, new imports, make code shorter/more readable
* variable revision number should go up by 1, it was 2 earlier * CellProfler Module class has a quirky way to introduce new class attributes via inheritance. This is not a Pythonic method to do things so an IDE should show a warning. * new version of CellProfiler makes one of the functions redundant, at least partially. The rest of it makes little sense any way.
* wx has been updated, libsubmit has been integrated into Parsl, Rynner has a slightly different project structure
Update README.md for this branch
Add numpy
+ Added function save_remote_pipeline to differentiate from save_pipeline, as alterations cause issues when saving locally.
+ Added option to save local copies of scripts in desired directory + Added option to set partition of job + Now reverts pipeline URLs after submission attempts to avoid irritating path changes in local pipeline + Corrected local and cluster save paths for pipeline batch data
+ Updated cellprofiler bash command to include singularity container preamble + Fixed DOS linebreak sanitising steps
+ Added a cluster_run_command function to allow for custom commands and command line arguments when running CellProfiler jobs. + Removed pipeline file location adjustments
+ Made adjustments to ClusterView to be able to see runs on cluster
+ Added download button to ClusterView module panel, and directory / subfolder options to fine-tune destination folder. + Removed RunStatusWindow popup + Added dialogs for download process
+ Moved functionality from RunOnCluster prepare_run function into other functions for ease of testing + Added parameterised tests for RunOnCluster + Added sanitise_scripts function to account for unneeded semi-colons
+ Some alterations to modules based on issues brought up in review meeting
+ Updated calls to Rynner and Libsubmit with specific git commit IDs
+ Updated sanitise_scripts to use semicolons as separator, improving reliability + Fixed test cases for test_group_images
+ Added cleanup lines to download function to remove temporary files when downloading + Included rudimentary testing of ClusterView settings + Added to do list with a couple of items
edbennett
requested changes
Jan 18, 2023
Member
edbennett
left a comment
edbennett
left a comment
There was a problem hiding this comment.
@TummyPan Clearly a lot of work has gone into these changes, thank you.
Some more tidying is needed for this to be ready for merge, I think. More details are inline in the attached review. Some issues I've highlighted the first time I saw them but occur across many files, so please be careful to check for all occurrences, not just the ones I've specifically called out.
Some general principles
- For future PRs, please try to avoid submitting PRs that mix large amounts of stylistic changes with bug fixes and other functionality changes, as the former make it very hard to see the latter
- Where changes are only stylistic and don't affect functionality, please consult on what the preferred style is before making them (and ideally rely on an auto-formatter to make them)
- Where functionality is being removed, remove it rather than leaving commented out blocks.
- Watch out for newlines at the end of files.
Member
|
For future reference: if you ever need to do a wide-ranging set of code formatting changes in another project, this should be done as a separate pull request from any changes to functionality. Mixing the two makes it very difficult for a reviewer to identify the changes in the PR that are relevant to the review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.