init code refactoring #742
Merged
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
Alexanderlacuna
force-pushed
the
chores/code-refactoring
branch
from
52cd4be
to
e41eec3
Compare
Alexanderlacuna
force-pushed
the
chores/code-refactoring
branch
from
2f6a482
to
5b58787
Compare
BonfaceKilz
reviewed
Nov 9, 2022
Comment on lines
79
to
80
| return not (float(trait.get('corr_coefficient', 0.0)) >= p_lower and | ||
| float(trait.get('corr_coefficient', 0.0)) <= p_upper) |
There was a problem hiding this comment.
A more pythonic (and readable way) to write this is:
Suggested change
| return not (float(trait.get('corr_coefficient', 0.0)) >= p_lower and | |
| float(trait.get('corr_coefficient', 0.0)) <= p_upper) | |
| return not (float(p_upper >= trait.get('corr_coefficient', 0.0)) >= p_lower) |
|
|
||
| def __min_filter__(min_expr): | ||
| if (target_dataset['type'] in ["ProbeSet", "Publish"] and target_trait['mean']): | ||
| return (min_expr != None) and (float(target_trait['mean']) < min_expr) |
There was a problem hiding this comment.
Similar comment to above:
Suggested change
| return (min_expr != None) and (float(target_trait['mean']) < min_expr) | |
| return float(target_trait['mean']) > min_expr != None |
There was a problem hiding this comment.
@BonfaceKilz the above example may lead to unintended results
for example
min_expr = None
return 2.4 < min_expr is not None
results to
TypeError: '<' not supported between instances of 'float' and 'str'and if you use bracket that means you will be comparing int to boolean
Comment on lines
93
to
100
| return ((location_chr != None and (target_trait["chr"] != location_chr) | ||
| or | ||
| (min_location_mb != None) and ( | ||
| float(target_trait['mb']) < min_location_mb) | ||
| or | ||
| max_location_mb != None) and | ||
| (float(target_trait['mb']) > float(max_location_mb) | ||
| )) |
There was a problem hiding this comment.
In this case too, you could chain operations together as demo'd above.
Comment on lines
102
to
109
| return ((location_chr != None and (target_trait["lrs_chr"] != location_chr) | ||
| or | ||
| (min_location_mb != None) and ( | ||
| float(target_trait['lrs_mb']) < float(min_location_mb)) | ||
| or | ||
| (max_location_mb != None) and ( | ||
| float(target_trait['lrs_mb']) > float(max_location_mb)) | ||
| ) |
There was a problem hiding this comment.
Same sentiment as above.
Comment on lines
278
to
281
| dataset_metadata = generate_table_metadata({name for trait in corr_results | ||
| for (name, _val) in trait.items()}, | ||
| correlation_data["traits_metadata"], | ||
| target_dataset_ob) |
There was a problem hiding this comment.
This nested "for" loop would be difficult to maintain. I'd advise using a more simpler, albeit verbose, loop instead.
|
Alexander Kabui ***@***.***>
aliandika:
@Alexanderlacuna commented on this pull request.
> @@ -74,6 +74,193 @@ def set_template_vars(start_vars, correlation_data):
return correlation_data
+def apply_filters(trait, target_trait, target_dataset, **filters):
+ def __p_val_filter__(p_lower, p_upper):
+ return not (float(trait.get('corr_coefficient', 0.0)) >= p_lower and
+ float(trait.get('corr_coefficient', 0.0)) <= p_upper)
+
+ def __min_filter__(min_expr):
+ if (target_dataset['type'] in ["ProbeSet", "Publish"] and target_trait['mean']):
+ return (min_expr != None) and (float(target_trait['mean']) < min_expr)
@BonfaceKilz the above example may lead to unintended results
for example
min_expr = None
``` python
return 2.4 < min_expr is not None
results to
TypeError: '<' not supported between instances of 'float' and 'str'
```
and if you use bracket that means you will be comparing int to boolean
You do have a point. Thanks for being keen on
this. That said, we have:
return 2.4 < min_expr is not None
which is [strictly] not [semantically] the same
as:
return 2.4 < min_expr != None
It's Worth pointing out that the "is" keyword
checks whether the objects are the same. You
generally don't want to use this most of the time.
OTOH, "!=" checks for equality of value - which
you want to use most of the time. Now on to the
important stuff:
Order matters.
Assuming "min_expr" is None, then in ...
return 2.4 < min_expr != None
... indeed an error, as you correctly pointed out,
will be generated (+1 for spotting that).
However, you can circumvent that using
short-circuiting by checking for the None value
first:
None != min_expr > 2.4
That said, in this particular case, for
readability's sake, in lieu of your observations,
opt for the simpler [original] case:
(min_expr != None) and (float(target_trait['mean']) < min_expr)
In conclusion, whichever expression you decide to
use is a matter of taste. Choose what you see
fit.
…--
(Life is like a pencil that will surely run out,
but will leave the beautiful writing of life.)
(D4F09EB110177E03C28E2FE1F5BBAE1E0392253F
(hkp://keys.openpgp.org))
|
Alexanderlacuna
force-pushed
the
chores/code-refactoring
branch
from
ee09525
to
dcab54a
Compare
|
@zsloan could try this out and see if there are any issues? |
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.
Description
How should this be tested?
Any background context you want to provide?
What are the relevant pivotal tracker stories?
Screenshots (if appropriate)
Questions