Conversation
Munchic
changed the title
Munchic
changed the title
TheAustinator
left a comment
TheAustinator
left a comment
There was a problem hiding this comment.
Really nice job making a straightforward developer interface for adding new plot methods and following the paradigms of the rest of the package
| def PLOT_METHODS(cls): | ||
| return cls.CONFIG["plot_methods"] | ||
| try: | ||
| plot_methods = cls.CONFIG["plot_methods"] |
There was a problem hiding this comment.
Another way to write this would be plot_methods = cls.CONFIG.get("plot_methods", parse_plot_methods(config=cls.CONFIG))
There was a problem hiding this comment.
Couldn't find parse_plot_methods, is this outdated?
dataforest/core/PlotMethods.py
Outdated
| try: | ||
| process_run = branch[branch.current_process] | ||
| plot_dir = process_run.plot_map[method_name].parent | ||
| plot_name = get_plot_name_from_plot_method( |
There was a problem hiding this comment.
Could probably be abstracted away to a _get_plot_dir method to avoid crowding the wrapper and having a lot of raw lines in the try block. Abstracting it away would also clarify what these lines are doing by virtue of the method name
There was a problem hiding this comment.
Seems like you resolved this!
dataforest/core/ProcessRun.py
Outdated
| @property | ||
| def process_plot_map(self) -> Dict[str, Path]: | ||
| return {file_alias: self.plots_path / self._plot_lookup[file_alias] for file_alias in self._plot_lookup} | ||
| plot_map_dict = {} |
There was a problem hiding this comment.
Docstring would be helpful
There was a problem hiding this comment.
Also looks like you broke it into several understandable steps
| from copy import deepcopy | ||
| import json | ||
| from collections import OrderedDict | ||
|
|
There was a problem hiding this comment.
Return value type hinting + brief description of what the keys/values represent (when the function returns a dict) would hugely clarify this file. A bit more of a detailed description in docstrings would be helpful as well
There was a problem hiding this comment.
Thanks, added some docstrings :)
Merge branch 'dev' into feature/qc-plots # Conflicts: # dataforest/core/DataBranch.py # dataforest/hooks/hooks/core/hooks.py
plotting debugging and notebook compatibility;
…ig; migrated groupby from cellforest; refactored subset and filter to allow `_MULTI_`; PlotMethods.{display, keys, methods}; PlotPreparator for stratify and faceting; improved config collectors; tether bug
Munchic
changed the title
2 participants
Updates
README.MDfrom CellForest for more details on architectureTesting
[x] Tested locally
[x] Tested notebook
[ ] Cleared notebook outputs[x] Docstrings and type-hinting
Fixes
Future works
Reviewer's tasks