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

[MRG] support loading of empty picklists #2106

Merged
merged 2 commits into from
Jul 7, 2022
Merged

Conversation

ctb
Copy link
Contributor

@ctb ctb commented Jul 6, 2022

Support allow_empty=True on SignaturePicklist.load(...).

Also adds a test_picklist.py file and tests/test-data/picklist/ directory, for future picklist tests.

Fixes #2096

@codecov
Copy link

codecov bot commented Jul 6, 2022

Codecov Report

Merging #2106 (685c1fd) into latest (50bc6dd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           latest    #2106   +/-   ##
=======================================
  Coverage   84.32%   84.32%           
=======================================
  Files         130      130           
  Lines       15281    15283    +2     
  Branches     2170     2171    +1     
=======================================
+ Hits        12885    12887    +2     
  Misses       2098     2098           
  Partials      298      298           
Flag Coverage Δ
python 91.78% <100.00%> (+<0.01%) ⬆️
rust 65.13% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/sourmash/picklist.py 91.50% <100.00%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50bc6dd...685c1fd. Read the comment docs.

@ctb
Copy link
Contributor Author

ctb commented Jul 6, 2022

Ready for review & merge @sourmash-bio/devs

@ctb
Copy link
Contributor Author

ctb commented Jul 7, 2022

Note: the rust checks that are failing have nothing to do with this PR. 🤷

@ccbaumler
Copy link
Contributor

I notice one other warning about mamba in the dev_env.yml:

mamba
Mamba support is still experimental and can result in differently solved environments!

Is this at all a concern?
Would it be best to specify the version of Mamba to use in the yml instead of the latest?

@ctb
Copy link
Contributor Author

ctb commented Jul 7, 2022

I notice one other warning about mamba in the dev_env.yml:

mamba
Mamba support is still experimental and can result in differently solved environments!

Is this at all a concern? Would it be best to specify the version of Mamba to use in the yml instead of the latest?

I actually don't find any references anywhere to a dev_env.yml - where are you seeing this? :)

In any case, if it's not related to either the functionality being implemented for the PR, or the changed files, then it's not relevant to the PR & you should start a new issue with that as a question, I think!

@ccbaumler
Copy link
Contributor

ccbaumler commented Jul 7, 2022

I created an issue like you suggested to investigate this warning further. See issue Mamba support is experimental 2109

The warning is located in any workflow that is under the Actions>Dev Env Instructions.

@ctb
Copy link
Contributor Author

ctb commented Jul 7, 2022

I created an issue like you suggested to investigate this warning further. See issue Mamba support is experimental 2109

The warning is located in any workflow that is under the Actions>Dev Env Instructions.

thx - yes, it was in dev_envs.yml under .github/workflows/, which was easier to find than dev_env.yml which does not exist :)

note you can refer to other issues in a few different ways:

all of these will reverse link that issue to here, which is nice!

Copy link
Contributor

@ccbaumler ccbaumler left a comment

Choose a reason for hiding this comment

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

LGTM

@ctb ctb merged commit 4029fdb into latest Jul 7, 2022
@ctb ctb deleted the dk/allow_empty_picklist branch July 7, 2022 19:21
@ctb
Copy link
Contributor Author

ctb commented Jul 7, 2022

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

consider supporting empty picklists in the picklist API
2 participants