-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes to sample data editing #781
base: testing
Are you sure you want to change the base?
Changes to sample data editing #781
Conversation
This doesn't yet support adding sample data for samples that don't already have values
Also fix the display so it only shows 3 decimal places
Deletions aren't working yet. This is dependent on a GN3 chance that allows the sample_list to be passed to get_trait_csv_sample_data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments; otherwise this LGTM.
@@ -171,7 +179,7 @@ def update_phenotype(dataset_id: str, name: str): | |||
or "" | |||
) | |||
phenotype_id = str(data_.get("phenotype-id")) | |||
if not (file_ := request.files.get("file")): | |||
if not (file_ := request.files.get("file")) and data_.get('edited') == "false": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: issue: This seems odd to me. Doing a string comparison the string "false". If there's a way to do directly with a boolean value, it would make this easier to both debug and read.
base_csv_lines = base_csv.split("\n") | ||
delta_csv_lines = [base_csv_lines[0]] | ||
|
||
for line in base_csv_lines[1:]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @zsloan @BonfaceKilz, maybe there's a way to code this that doesn't have a high cyclomatic complexity?
Otherwise, #781 LGTM
Feel free to ignore if you think this is a nitpick...
I also really don't like how those nested for loops work, but I'm not sure
how else to do them. The original version of the code didn't have this
problem because it only needed to care about samples with values (which
would be the same in both the base and delta CSV files) and also had the
CSV directly submitted instead of specific values being submitted through a
form.
Basically what needs to happen is for a CSV string to be written that is
essentially the base_csv but with values substituted when they were edited
(what it's getting from the form). So it goes through each line and checks
if edits of the value, error, or n_cases were submitted with the form.
It could maybe be made a bit more simple by converting all the edits into a
single JSON value either before or after being submitted with the form,
though this would also require more code and it would still need to check
"do edits exist" for each sample/row. I think this would only circumvent
the "if form_data.get()" checks.
…On Fri, May 12, 2023, 10:46 AM jgart ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In wqflask/wqflask/metadata_edits.py
<#781 (comment)>
:
> @@ -705,3 +727,26 @@ def approve_data(resource_id: str, file_name: str):
"warning",
)
return redirect(url_for("metadata_edit.list_diffs"))
+
+def create_delta_csv(base_csv, form_data, sample_list):
+ base_csv_lines = base_csv.split("\n")
+ delta_csv_lines = [base_csv_lines[0]]
+
+ for line in base_csv_lines[1:]:
Hi, maybe there's a way to code this that doesn't have such a high
cyclomatic
***@***.***/python-code-complexity-metrics-e6c87646c1c2>
complexity?
Otherwise #781 <#781> LGTM
—
Reply to this email directly, view it on GitHub
<#781 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANQJGAS7VMFUMFLOQQCN4LXFZLMDANCNFSM6AAAAAAX6Q4CWE>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
04f10df
to
5474e66
Compare
This PR will make a couple changes to sample data editing:
The corresponding GN3 PR is here - genenetwork/genenetwork3#117