Refactor find_spots and import to not depend on __main__, deprecate ImageImporter #2080
Conversation
This file contains 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
The previous onion-shaped import class has been kept but marked as deprecated
Can't return be default because this causes extra output from console_scripts. Tests currently rely on this having a return value, and the do_find_spots doesn't take "argument-like" arguments.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Both
dials.find_spotsanddials.importcheck if they are imported as__main__to decide whether to configure logging (and thus show any output). This assumption fails if running from a standardconsole_scriptsentry point.This refactors both scripts away from the legacy
a = Script(); a.run()pattern more into the style of https://github.com/dials/dials/blob/main/doc/examples/boilerplate.py, which advocates splitting therun()method from ado_Xmethod that assumes the logging is already set up.Import was... slightly complex here - because the heavy lifting for import is implicitly done when parsing the arguments, and it does a first pass to determine the logging level, the function is quite intertangled with the ArgumentParser flow. Thus, I've added a
configure_logging=function parameter which toggles this. I've kept but deprecated theImageImporterclass because it seems more likely that this is used elsewhere. I've updated the tests that used this directly.There are also tests that assume that the
find_spots.runfunction returns the experiments and reflection table, and passes in argument-style parameters. For this reason,find_spots.runhere returns if passedreturn_results=True. The tests should probably be refactored into running as-a-subprocess for this.