search

ucsd-ccbb / qiimp

Web application to collect metadata specifications from an experimenter and produce metadata input files with appropriate constraints
3 stars 7 forks source link

Make default units 'years' for host_age and 'cm' for host_height #148

Closed adswafford closed 5 years ago

adswafford commented 5 years ago

Original issue title: host_age_units, host_height_units, and host_weight_units should not restrict to missing: not provided

'missing: not provided' has been replaced by 'not provided' and these values cannot be changed to put in any useful information.

adswafford commented 5 years ago

Corrected issue to clarify that host_age is not affected, but host_height_units, and host_weight_units are

AmandaBirmingham commented 5 years ago

There are two things going on here, one of which is easy to fix and the other of which is not because it is working as it was built to do.

1) Someone typed in ' missing: not provided' as the units for host_height and host_weight (see screenshots at end); it is easy to change this to some specific other value. 2) The contents of the units columns are not changeable. This is because that is not what the automatic units columns were built to do; the spec for the unit column was: "No units for continuous date. For other continuous, units NOT optional. Required single value. Generate new column ALWAYS filled with that value." (see https://github.com/ucsd-ccbb/qiimp/issues/2 ).

If you want units columns that can have different values for different rows, then I see three options, two of which are hacky and the other of which is complicated: 1) Change the schema in host_associated.xlsx directly to make the auto-generated unit column a string with no restrictions on it. Will work, but the next time we open host_associated.xlsx in QIIMP to change something else, it will overwrite the manual change. 2) Turn off the requirement in QIIMP that user-defined column names can't have "_units" at the end of them. Define "host_height" and "host_weight" as unitless columns, and ALSO define two new columns, "host_height_units" and "host_weight_units" that can hold any string you want. 3) Work out specifications to indicate when an auto-generate units columns should contain only a single, unchangeable value and when it should allow arbitrary values, figure out how to display this in the interface for users to select, and then implement it in the codebase.

Let me know which you want to pursue, or if you can think of another alternative!

screen shot 2018-08-29 at 12 37 08 pm

screen shot 2018-08-29 at 12 37 40 pm

adswafford commented 5 years ago

"Someone typed in ' missing: not provided' as the units for host_height and host_weight (see screenshots at end); it is easy to change this to some specific other value."

Good news then; we just need to fix the template. Assigning to @stephanieorch to update it and to check the other packages for similar issues. The default units should be 'years' for host_age and 'cm' for host_height. @stephanieorch let me know if you're not sure on the units for any others you come across that currently have null values for units.

AmandaBirmingham commented 5 years ago

@adswafford Cool! I'm just double-checking this in case it is too good to be true :) ... What Stephanie enters won't be "default" units, they will be the ONLY allowable units; e.g., if we put "years" as the units for host_age, then "years" will be the ONLY value that passes validation for the host_age_units field. If this is what you want, great! If not, we should discuss more.

stephanieorch commented 5 years ago

Please note that this issue should only be applied to the "host-associated" package. So humans, rats, and mice already have the correct units applied to them.

Let me know what I should switch "missing: not provided" to.

AmandaBirmingham commented 5 years ago

@adswafford what do you want the units for host_weight_units in the host-associated package to be? I've put in kg for the moment but need your verification.

Also, I see that host_age, host_height, and host_weight are defined in the host-associated package but then RE-defined in the human package. As far as I can tell, the difference between the definitions is that in the host-associated package, these fields can be any of the missing values, while in the human package, they cannot be any of the missing values. Is this a desired difference, or can the two packages share a single definition of these fields?

adswafford commented 5 years ago

kg is great for host_weight_units. For the host_age, host_height, and host_weight these should be able to be "not collected" or "not provided" for all of these in the both host-associated package and the human package.

However host_age_units should be 'years', host_height_units = 'cm', and host_weight_units = 'kg' for the human-associated, but not restricted for the host-associated.

AmandaBirmingham commented 5 years ago

"However [the three units columns] should be ... not restricted for the host-associated"

@adswafford , this is not an option in QIIMP. Please see previously provided descriptions https://github.com/ucsd-ccbb/qiimp/issues/148#issuecomment-417082218 and https://github.com/ucsd-ccbb/qiimp/issues/148#issuecomment-417148513 above: the contents of the automatic units columns are not changeable. Rather, they can only contain a single value that is defined at template creation time, per the original specification for that type of column.

I provided three options (none of them particularly appealing) for changing the behavior of the automatic units columns in the first cited comment above. If you feel we need to pursue one of these, you and I should discuss it further.

adswafford commented 5 years ago

Ok, let's make the host-associated units the same as the ones for human-associated for now and see if we get complaints.