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 count_atoms to GrammaticalExpression #48

Merged
merged 2 commits into from
Nov 20, 2024
Merged

Conversation

Ashvin-Ranjan
Copy link
Contributor

Notes

  • When implementing the test in test_grammar.py, numerous issues with testing came up
    • test_grammar.py and test_language.py both have issues with Universe expecting a tuple of priors
    • test_language.py also has some issues with a Meaning being initialized incorrectly
    • test_meaning in test_grammar.py fails (even after changes are reverted)
    • However after implementing temporary workarounds, test_atom_count does pass
  • As mentioned in GrammaticalExpression _len_ does not return number of atoms / leaf nodes #42. count_atoms can also be moved to a helper method in grammar.py, I simply felt it was more in line with the current project structure to make it a method of the GrammaticalExpression class

- Adds the method `count_atoms` to `GrammaticalExpression`
- This also adds a proper test for the method in `test_grammar.py`
Copy link
Collaborator

@shanest shanest left a comment

Choose a reason for hiding this comment

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

Thanks, Ash! Much appreciated. One small change requested, just for readability :)

Re your point about testing: the tests are indeed quite outdated (we made some breaking changes to (at least) Universe and Meaning since they were written). If you're interested in digging more into ULTK, going through and cleaning up the test suite would be very welcome!

length = 0
if self.children is not None:
length += sum(child.count_atoms() for child in self.children)
return length
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! This can be streamlined slightly: after the original if statement on line 177, can just do return sum(child.count_atoms() for child in self.children) :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! I copied the code over from __len__ without realizing that it was a if statement, not a for loop, I have pushed a correction. Thank you for pointing it out.

@Ashvin-Ranjan
Copy link
Contributor Author

Thank you for looking over the PR! I have implemented the suggestion you have made.

Regarding unit tests, I would be more than happy to look into them and clean them up as a way to familiarize myself more with ULTK as a whole.

Copy link
Collaborator

@shanest shanest left a comment

Choose a reason for hiding this comment

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

Awesome; thanks for this and for taking a look at the tests next!

@shanest shanest merged commit d915934 into CLMBRs:main Nov 20, 2024
@Ashvin-Ranjan Ashvin-Ranjan mentioned this pull request Nov 20, 2024
jingnongqu pushed a commit to jingnongqu/ultk that referenced this pull request Jan 26, 2025
Add `count_atoms` to `GrammaticalExpression`
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.

2 participants