add methods to export results in tabular format #280
Conversation
liannette
commented
Oct 16, 2024
liannette
commented
- add method to nplinker class for exporting links, as well as genomic and metabolomic data
- add test for generating the tabular data for the links
… file, improve docstrings
|
Please assign me to review it when it's ready ;-) If you're still working on that, it's better to change it to a draft PR |
|
I thought I was finished, but I realized that it still needs a bit of work. I will assign you as soon as I'm happy with it! |
|
@CunliangGeng It's ready for review :) I just can not request a review explicitly, because I have only read permission for the repository. |
There was a problem hiding this comment.
Let's check the code step by step, and first thing to check is about code format and style:
- Please check the errors in the static typing, and correct the incorrect typings. You could use
mypyto check them locally before committing. - It is not explicitly mentioned, but we do follow some rules for the order of methods/functions in a class/file. The order of methods/functions are:
__init__method- other magic methods, e.g.
__str__ - property methods (using
@property) - regular methods, class methods (using
@classmethod), static methods (using@staticmethod) - private methods (
_func) - deprecated methods (using
@deprecated)
For the same level of methods, e.g. regular methods, it's recommended to order them in alphabetical order.
Please check the new methods/functions you added and put them in the right place.
|
Is there anything more needed before merging? |
I'll take a closer look today. |
Apply suggestions from code review Co-authored-by: Cunliang Geng <[email protected]>
Apply suggestions from code review Co-authored-by: Cunliang Geng <[email protected]>
…into output_files
|
Done! I've integrated your suggestions or explained my reasoning for any differences. I hope we're good to merge now. |
|
Okay, it's implemented! |
| # Convert dict to comma-separated string | ||
| elif isinstance(value, dict): | ||
| value = ", ".join([f"{k}:{v}" for k, v in value.items()]) | ||
| # Convert anything else to string |
There was a problem hiding this comment.
Do you really want to covert None to ""? Does it make sense to the BGC attributes?
There was a problem hiding this comment.
If the value is None, the corresponding field in the tabular output file should be left empty. This ensures that when the file is opened in Excel, numeric fields are correctly recognized as numbers rather than text, allowing the columns to be sorted properly. For text fields, leaving them empty is also preferable to displaying None, as it is cleaner and more intuitive.
…, tabs are replaced in to_tabular
|
Alright, I really hope that we can merge this now 👀 |
There was a problem hiding this comment.
I also hope we can merge it now 
However, we must adhere to some important quality requirements, such as making docstrings and comments accurate and clear; and adding unit tests for all public unit tests. These quality requirements will benefit everyone over time, rather than accumulating more and more technical debt
You could choose what to do next:
1 🍎 Continue this PR and resolve the remaining comments
2 🍐 Merge this PR and open some new and small PRs to resolve the remaining comments
Let me know which one you prefer.
| @@ -225,13 +225,16 @@ def to_string(value: Any) -> str: | |||
| value = str(value) if value else "" | |||
| return value | |||
|
|
|||
| def to_tabular(self) -> list: | |||
| def to_tabular(self) -> dict[str, Any]: | |||
There was a problem hiding this comment.
| def to_tabular(self) -> dict[str, Any]: | |
| def to_tabular(self) -> dict[str, str]: |
| """Convert various types of values to a string. | ||
|
|
||
| Args: | ||
| value: The value to be converted to a string. | ||
| Can be a list, tuple, set, dict, or any other type. | ||
| Can be a list, dict, or any other type. |
There was a problem hiding this comment.
| Can be a list, dict, or any other type. | |
| Can be a list, dict, or other JSON-compatible types. |
| """Convert various types of values to a string. | ||
|
|
||
| Args: | ||
| value: The value to be converted to a string. | ||
| Can be a list, tuple, set, dict, or any other type. | ||
| Can be a list, dict, or any other type. | ||
|
|
||
| Returns: | ||
| A string representation of the input value. | ||
| """ | ||
| # Convert list, tuple, set to comma-separated string |
There was a problem hiding this comment.
| # Convert list, tuple, set to comma-separated string | |
| # Convert list to comma-separated string |
| """Convert the BGC object to a tabular format. | ||
|
|
||
| Returns: | ||
| list: A list of strings representing the BGC object in tabular format. | ||
| dict: A dictionary representing the BGC object in tabular format. |
There was a problem hiding this comment.
| dict: A dictionary representing the BGC object in tabular format. | |
| dict: A dictionary representing the BGC object in tabular format. | |
| The keys can be treated as headers and values are strings in which tabs are removed. | |
| This dict can be exported as a TSV file. |
There was a problem hiding this comment.
It's required to test public methods to_tabluar and optional to test private method _to_string. So please add unit tests for the public methods.
There was a problem hiding this comment.
It's required to test public methods to_tabluar and optional to test private method _to_string. So please add unit tests for to_tabluar method.
There was a problem hiding this comment.
Please update the docstrings and comments based on my comments in bgc.py
There was a problem hiding this comment.
Please add unit tests for public method to_tsv.
There was a problem hiding this comment.
Please add unit tests for the new public methods objects_to_tsv and to_tsv in test_nplinker_local.py.
