Edinburgh-Genome-Foundry / DnaChisel

:pencil2: A versatile DNA sequence optimizer
https://edinburgh-genome-foundry.github.io/DnaChisel/
MIT License
213 stars 38 forks source link

Add comparison operators to Location object #72

Closed godotgildor closed 1 year ago

godotgildor commented 1 year ago

I was writing some unit tests for a tool I wrote which uses DnaChisel. I wanted to assert equality between some Location objects, but found that currently Location only supports < and it looks like it tries to support >=.

This PR does the following:

veghp commented 1 year ago

Thanks for the contribution! Good to hear you found DNA Chisel useful. What is the purpose of your tool / is it public by any chance?

The mentioned extra dependencies are not required for the package, we use it only for testing & development. They're installed during CI: https://github.com/Edinburgh-Genome-Foundry/DnaChisel/blob/58321ce018318935ee28f448b2c051d4b00f370e/.travis.yml#L7

If you wish to include them, please create a requirements-dev.txt file in root and list them there instead.

I'll try and have a look at the rest sometime but could you please provide context for your feature requirement; also just to confirm >= does not work?

Zulko commented 1 year ago

My bad for __geq__. Regarding test dependencies, another way could be to add a tests section to the extra_requires here so you can do pip install dnachisel[tests]. But a requirements-dev.txt also works.

godotgildor commented 1 year ago

My bad for __geq__. Regarding test dependencies, another way could be to add a tests section to the extra_requires here so you can do pip install dnachisel[tests]. But a requirements-dev.txt also works.

I went ahead and made a tests extra that includes all of the packages installed in the travis ci currently. I also updated the travis CI config to use this extras install.

godotgildor commented 1 year ago

@Zulko - just wanted to check back-in about this PR - anything else I can provide or changes you'd like to see?

veghp commented 1 year ago

I'm merging this today or tomorrow together with the other 2 PRs

veghp commented 1 year ago

Unfortunately the tests fail for EnforceRegionsCompatibility because Location became unhashable due to the changes. Location.__hash__ attribute is now set to None. This means the Counter function doesn't work on a list of Locations:

https://github.com/Edinburgh-Genome-Foundry/DnaChisel/blob/58321ce018318935ee28f448b2c051d4b00f370e/dnachisel/builtin_specifications/EnforceRegionsCompatibility.py#L32

Suggestions are welcome. Perhaps a solution is to implement a simple counter, for example https://stackoverflow.com/questions/45955740/counting-occurrences-without-using-collections-counter

https://github.com/Edinburgh-Genome-Foundry/DnaChisel/actions/runs/5164168843/jobs/9302899033

godotgildor commented 1 year ago

I just updated the PR with a commit which adds in a __hash__ method for Location and corresponding tests.

I think this should address the issue with the Counter.

Can you take a look?

veghp commented 1 year ago

Thank you, that was helpful. Tests pass, so I'll make a release by tomorrow after merging the other PR.