Final Project 3 #5
Conversation
There was a problem hiding this comment.
Overall, nicely done! Your code is functional and well-documented. There are some stylistic suggestions I've made in this comment. It would be ideal to implement all the functions as fruitful functions because they allow you to write unit tests. And please try to avoid using a global variable.
text_mining.py
Outdated
| 'New Mexico', 'New York (state)', 'North Carolina', 'North Dakota', 'Ohio', | ||
| 'Oklahoma', 'Oregon', 'Pennsylvania', 'Rhode Island', 'South Carolina', | ||
| 'South Dakota', 'Tennessee', 'Texas', 'Utah', 'Vermont', | ||
| 'Virginia', 'Washington (state)', 'West Virginia', 'Wisconsin', 'Wyoming'] |
There was a problem hiding this comment.
If you were feeling really really lazy, you could've used Wikipedia to retrieve these states( and the distances) as well! You can use Pickle to save those data so that you don't send GET requests to Wikipedia everytime you are running this code
text_mining.py
Outdated
| # Orders by frequency without repeat terms | ||
| seen = {} | ||
| ordered_set = [seen.setdefault(x, x) for x in ordered if x not in seen] | ||
| freq_words.append(ordered_set) |
There was a problem hiding this comment.
nice way of using dictionary to remove duplicates!
and 👍 for the list comprehension
text_mining.py
Outdated
| # Delete words that appear in 10 or more articles | ||
| for word in hist.keys(): | ||
| if hist[word] >= 10: | ||
| for i in range(50): |
There was a problem hiding this comment.
It would be helpful if you add more comments for this function. I don't understand why you are running this 50 times just from reading your comments. Where is this magic 50 coming from?
text_mining.py
Outdated
| find_freq_words(name) | ||
| print(name) | ||
|
|
||
| remove_matches() |
There was a problem hiding this comment.
From my understanding, this function should only return common words from state 1 and 2. I recommend you move remove_matches() outside this function to better modularize your code.
text_mining.py
Outdated
| seen = {} | ||
| ordered_set = [seen.setdefault(x, x) for x in ordered if x not in seen] | ||
| freq_words.append(ordered_set) | ||
| return |
There was a problem hiding this comment.
This is a void function that uses a global variable. Generally, I would recommend implementing a fruitful function instead that does not rely on a global variable. Please refer to this StackOverflow article to find why https://stackoverflow.com/questions/19158339/why-are-global-variables-evil
text_mining.py
Outdated
| for i in range(50): | ||
| if word in freq_words[i]: | ||
| freq_words[i].remove(word) | ||
|
|
There was a problem hiding this comment.
again, void function vs fruitful function. https://stackoverflow.com/questions/19158339/why-are-global-variables-evil
text_mining.py
Outdated
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| comp_al() |
There was a problem hiding this comment.
It would be easier to follow your code if you had all the function executed here in if __name__ == "__main__":. Please Check out this StackOverFlow article as well. https://stackoverflow.com/questions/19158339/why-are-global-variables-evil
text_mining.py
Outdated
|
|
||
| import wikipedia | ||
| from collections import Counter | ||
| from itertools import chain |
There was a problem hiding this comment.
I don't see where you are using this chain. If you are not using this library anymore, please delete it from your final code!
|
Nicely done revision! |
No description provided.