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

Add comparison operators to Location object #72

Conversation

godotgildor
Copy link

@godotgildor godotgildor commented Mar 24, 2023

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:

  • Adds in two missing dependencies for DnaChisel that I discovered when I tried to run the DnaChisel test suite. (Feel free to let me know if I missed them and I can remove this part from the PR)
  • Adds an __eq__ method to the Location class so that we can test for equality now.
  • Removes the __geq__ method - I believe the correct syntax would have been __ge__. However we don't need this method now because I also:
  • Added the total_ordering decorator to the definition of the Location class. This decorator will automatically create all comparison operators as long as the class has a definition of __eq__ and one other comparison - in this case we also have __lt__
  • Added some unit tests for Location to check that the ==, >= and < comparisons all work.

@veghp
Copy link
Member

veghp commented Mar 27, 2023

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:

- pip install matplotlib primer3-py genome_collector geneblocks

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
Copy link
Member

Zulko commented Mar 27, 2023

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
Copy link
Author

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
Copy link
Author

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

@veghp
Copy link
Member

veghp commented Jun 2, 2023

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

veghp added a commit that referenced this pull request Jun 3, 2023
@veghp
Copy link
Member

veghp commented Jun 3, 2023

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:

counter = Counter(all_locations_with_incompatibility)

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
Copy link
Author

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 veghp changed the base branch from master to dev June 5, 2023 15:50
@veghp veghp merged commit 68f993d into Edinburgh-Genome-Foundry:dev Jun 5, 2023
@veghp
Copy link
Member

veghp commented Jun 5, 2023

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

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.

3 participants