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.

Sign up for GitHub

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

Jump to bottom

ENH: Added new type FeatureData[SequenceCharacteristics] with semantic validator for property 'length'. #326

Merged
merged 8 commits into from
May 16, 2024
Merged

ENH: Added new type FeatureData[SequenceCharacteristics] with semantic validator for property 'length'. #326

merged 8 commits into from
May 16, 2024

Conversation

VinzentRisch
Copy link
Contributor

@VinzentRisch VinzentRisch commented May 8, 2024 •
edited
Loading

closes #324

  • Added new type FeatureData[SequenceCharacteristics].
  • Added new semantic validator for FeatureData[SequenceCharacteristics % Properties("length")].

install this branch

git clone https://github.com/qiime2/q2-types.git
cd q2-types
git fetch origin pull/326/head:pr-326
git checkout pr-326
pip install -e .

test it out
sequence_characteristics.txt

qiime tools import --input-path sequence_characteristics.txt --output-path  sequence_characteristics.qza --type "FeatureData[SequenceCharacteristics % Properties('length')]"

@VinzentRisch VinzentRisch marked this pull request as draft May 8, 2024 13:26
@VinzentRisch VinzentRisch force-pushed the 324_featuredata_sequence_characteristics branch from e685a54 to 5d57bb2 Compare May 8, 2024 14:19
@VinzentRisch VinzentRisch marked this pull request as ready for review May 8, 2024 14:26
misialq
misialq requested changes May 13, 2024
View reviewed changes
Copy link
Collaborator

@misialq misialq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @VinzentRisch, this mostly looks good to me - just one "small" request below. Thanks!

q2_types/feature_data/_transformer.py Show resolved Hide resolved
q2_types/feature_data/_type.py Outdated Show resolved Hide resolved
@misialq misialq mentioned this pull request May 13, 2024
Closed
@misialq misialq mentioned this pull request May 14, 2024
Merged
@misialq
Copy link
Collaborator

misialq commented May 14, 2024

Hey @lizgehret, @ebolyen! This is the PR for the new type we were discussing in our last dev meeting - would one of you maybe have some time to check it out? 🙏

@lizgehret lizgehret self-requested a review May 14, 2024 20:57
@lizgehret lizgehret self-assigned this May 14, 2024
@lizgehret @misialq
Update q2_types/feature_data/_format.py
d37df0c 
Co-authored-by: Michal Ziemski <mziemski@ethz.ch>
lizgehret
lizgehret reviewed May 15, 2024
View reviewed changes
Copy link
Member

@lizgehret lizgehret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall this looks great - some veeery small/nitpicky comments (sorry!) but w/those suggestions it should be ready to merge!

q2_types/feature_data/_transformer.py Outdated Show resolved Hide resolved
q2_types/feature_data/_transformer.py Outdated Show resolved Hide resolved
q2_types/feature_data/_type.py Outdated Show resolved Hide resolved
q2_types/feature_data/tests/test_format.py Outdated Show resolved Hide resolved
q2_types/feature_data/tests/test_format.py Outdated Show resolved Hide resolved
q2_types/feature_data/tests/test_format.py Outdated Show resolved Hide resolved
q2_types/feature_data/tests/test_format.py Outdated Show resolved Hide resolved
q2_types/feature_data/tests/test_format.py Outdated Show resolved Hide resolved
q2_types/feature_data/tests/test_transformer.py Outdated Show resolved Hide resolved
q2_types/feature_data/tests/test_type.py Outdated Show resolved Hide resolved
@lizgehret lizgehret assigned VinzentRisch and unassigned lizgehret May 15, 2024
VinzentRisch and others added 3 commits May 16, 2024 10:09
@VinzentRisch @lizgehret
Update q2_types/feature_data/_transformer.py
9916d10 
Co-authored-by: Liz Gehret <54517601+lizgehret@users.noreply.github.com>
@VinzentRisch @lizgehret
Apply suggestions from code review
6eea912 
Co-authored-by: Liz Gehret <54517601+lizgehret@users.noreply.github.com>
@VinzentRisch
changed names to tsv, cleanup
19e3053
@VinzentRisch
Copy link
Contributor Author

Hi @lizgehret
Thanks for the review. 🙂
I now changed all quotes to be consistent with the other code.

@VinzentRisch VinzentRisch requested review from lizgehret and misialq May 16, 2024 08:32
misialq
misialq approved these changes May 16, 2024
View reviewed changes
Copy link
Collaborator

@misialq misialq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good on my side, thanks @VinzentRisch! Could you please still resolve the merge conflict?

@lizgehret
Copy link
Member

changes lgtm, thanks @VinzentRisch and @misialq!

@lizgehret lizgehret merged commit 806def6 into qiime2:dev May 16, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lizgehret lizgehret lizgehret approved these changes

@misialq misialq misialq approved these changes

Assignees
No one assigned
Labels
None yet
Projects
No open projects
2024.5
Status: Completed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ENH: Add new type FeatureData[SequenceCharacteristics]
3 participants
@VinzentRisch @misialq @lizgehret