Skip to content
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

[traits.build workflow] Add "time" category of traits, in addition to "categorical" and "numeric"? #87

Open
yangsophieee opened this issue Oct 6, 2023 · 1 comment
Labels
enhancement New feature or request

Comments

@yangsophieee
Copy link
Collaborator

yangsophieee commented Oct 6, 2023

Currently dataset_test fails when substitutions for time variables (e.g. flowering_time, fruiting_time) exist, because of this line looking for whether values are found within the trait definitions.

I can think of several easy ways to fix this: if the values only contain "n" and "y" then you don't check for the trait definitions (implemented here).

This made me think about this line in process.R which specifically codes for the traits flowering_time and fruiting_time. But this isn't very generalisable to other databases if they want to make their own time traits. I wonder if it's worth making a new "time" category of traits, in addition to "categorical" and "numeric". What do you think @ehwenk?

@yangsophieee yangsophieee changed the title dataset_test picks up time values as unallowed Add "time" category of traits, in addition to "categorical" and "numeric"? Oct 6, 2023
@ehwenk
Copy link
Collaborator

ehwenk commented Oct 9, 2023

I've previously thought about having a separate trait category for flowering_time, etc. for similar reasons. Because when I've added a new time-trait (recruitment_time), you're right, I then had to tweak code (I think for building the dictionary ontology). And it probably should have been added to this list as well.

The specific test you referenced searches for N's and Y's only, which is specific to time.

But a separate problem with the time traits is that they are categorical traits for which the allowable trait values aren't listed. That could be true to non-time traits - given the trait recruitment_time isn't causing any errors, it seems we've already adapted the traits.build workflow to handle instances where categorical traits don't have set values. Maybe this is actually only a problem for the code to build the APD, in which case I guess it is fine to require a fix.

@dfalster what do you think? Is it a time category we should consider adding or a factor (or other word) category for traits without set values. Or nothing?

@ehwenk ehwenk added the on-hold label Oct 12, 2023
yangsophieee added a commit that referenced this issue Oct 12, 2023
- Fixes #49
- Add test for substituting a time value
- Update context categories for `metadata_add_contexts`
- Convert context values to character type, preventing time data types from converting weirdly when converted to yaml in `write_metadata`
- Remove "time" values from `dataset_test` checks of allowable categorical values
- Add a note to the user when time data types get reformatted
- Partial solution to #87 where substitutions for time variables (e.g. `flowering_time`, `fruiting_time`) fail `dataset_test`
@ehwenk ehwenk added the enhancement New feature or request label Oct 16, 2023
@ehwenk ehwenk changed the title Add "time" category of traits, in addition to "categorical" and "numeric"? [traits.build workflow] Add "time" category of traits, in addition to "categorical" and "numeric"? Jul 31, 2024
@ehwenk ehwenk removed the on hold label Jul 31, 2024
@ehwenk ehwenk added this to AusTraits Jul 31, 2024
@ehwenk ehwenk moved this to Backlog in AusTraits Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Backlog
Development

No branches or pull requests

2 participants