-
Notifications
You must be signed in to change notification settings - Fork 61
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
Evaluator updates #103
base: master
Are you sure you want to change the base?
Evaluator updates #103
Conversation
- minor bug fixes in evaluation - added the ability to use generic entities and skip words - added the ability to do faster batch predict - removed the old CRF implementation - added sample_id to be able to reproduce the full sample from error
- minor bug fixes in evaluation - added the ability to use generic entities and skip words - added the ability to do faster batch predict - removed the old CRF implementation - added sample_id to be able to reproduce the full sample from error - fixed issue with hospital provider networking
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Omri, great PR! Thanks for working on it. I’ve run it locally, and it works perfectly. I just have a few minor comments.
"""Graph most common false positive and false negative tokens for each entity.""" | ||
ModelError.most_common_fp_tokens(self.errors) | ||
fps_frames = [] | ||
fns_frames = [] | ||
for entity in self.model.entity_mapping.values(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you have identified the 'Wrong-Entity' error, I'm not sure if it makes sense to include a plot for it. Should we add a plot for the most common 'Wrong-Entity' errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it's part of FPs. Would you suggest to separate the two? or have it as part of FPs and wrong entity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plot_most_common_tokens function calls different functions depending on the type of error it is analyzing. For false positive errors, it calls the get_fps_dataframe function with the parameter error_type="FP". For false negative errors, it calls the get_fns_dataframe function with the parameter error_type='FN'. Additionally, to analyze errors related to the wrong entity, it calls the get_wrong_entity_dataframe function with the parameter error_type="Wrong entity"
return ((1 + beta**2) * precision * recall) / ( | ||
((beta**2) * precision) + recall | ||
) | ||
return ((1 + beta**2) * precision * recall) / (((beta**2) * precision) + recall) | ||
|
||
class Plotter: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could separate classes Evaluation and Plotter because it enhances maintainability and readability, especially since the file is lengthy at 727 lines. It's also easier for testing and reuse of each class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea!
fig.update_traces(textfont=dict(size=10)) | ||
fig.update_layout(width=800, height=800) | ||
|
||
fig.show(save_as) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe adding error handling here to handle unexpected value of save_as paramter?
) | ||
fig.update_layout(yaxis={"categoryorder": "total ascending"}) | ||
fig.show() | ||
fig.show(save_as) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe adding error handling here to handle unexpected value of save_as paramter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I'm thinking in the automated pipeline, the end-user might prefer to save the plot as a file rather than visualizing it directly. Therefore, it could be beneficial to include an option like fig.write_image('filename.png')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One way to do this is just to return the fig
object, which allows the user to take it and save it. Another is to add a parameter with a path to save in. Which one would be better in your perspective?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to return the fig object.
Revamp of the evaluation process, including: