Feature/corerelation refactoring #647
Conversation
There was a problem hiding this comment.
@Alexanderlacuna make some changes based off things I remarked. Let's address some of the other issues, like how you check the admin_access_role later ok?
| @@ -49,6 +49,7 @@ | |||
| import codecs | |||
|
|
|||
| import json | |||
| import ijson | |||
There was a problem hiding this comment.
issue: Submit a patch for python-ijson to me or @arunisaac in our channel before we merge, otherwise this will cause gn2 to fail...
| @@ -1311,7 +1311,8 @@ def fetch_cached_results(dataset_name: str, dataset_type: str): | |||
| file_path = os.path.join(TMPDIR, f"{file_name}.json") | |||
| try: | |||
| with open(file_path, "r") as file_handler: | |||
| # return json.load(file_handler) | |||
| return [obj for obj in ijson.items(file_handler, "", use_float=True)][0] | |||
There was a problem hiding this comment.
question: Wouldn't it be better if we returned an iterator, instead of a list?
| """params:file_path | ||
| return:size of file in MB |
There was a problem hiding this comment.
issue: I'll raise this in tomorrow's stand-up or on e-mail; but we need to come up with a standard way of writing doc-strings.
| @@ -156,3 +160,39 @@ def get_datasets_data(base_dataset, target_dataset_data): | |||
| samples_fetched, base_traits_data) | |||
|
|
|||
| return (target_results, base_results) | |||
|
|
|||
|
|
|||
| def get_file_size(file_path): | |||
There was a problem hiding this comment.
issue: AFAICT, this function is only used once. No need defining functions that are only used once. Also, we just checking the file size, which can be done in 1 line:
import os
os.stat("<file/patch>").st_size / (1024 ** 2) # in Mb
| streamed_json = [obj for obj in ijson.items( | ||
| file_handler, prefix, use_float=True)] |
There was a problem hiding this comment.
issue: question: shouldn't we use an iterator for this as well? We use ijson, we is an iterator, and then convert it into a list, which beats the point of using ijson in the first place.
| def generate_button_configs(environ="production"): | ||
| user_id = ((g.user_session.record.get(b"user_id") or b"").decode("utf-8")) | ||
|
|
||
| try: | ||
| user_role = get_highest_user_access_role( | ||
| user_id=user_id, resource_id="", gn_proxy_url=GN_PROXY_URL)["admin"] | ||
|
|
||
| except Exception: | ||
| user_role = False | ||
|
|
||
| return True if (user_role or environ != "production") else False |
There was a problem hiding this comment.
issue: There has to be a better way to do this. Let's discuss tomorrow(or remind me).
| {% include 'show_trait_calculate_correlations.html' %} | ||
| {% with show_test_button= display_test_button %} | ||
|
|
||
| {% include 'show_trait_calculate_correlations.html' %} | ||
| {% endwith %} | ||
|
|
| {% if show_test_button is sameas true %} | ||
| <input type="button" class="btn test_corr_compute submit_special btn-success" data-url="/test_corr_compute" title="Compute Correlation" value="Test Compute"> | ||
| {% endif %} |
fredmanglis
force-pushed
the
testing
branch
2 times, most recently
from
04f10df
to
5474e66
Compare
PR works on correlation refactor