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

Plot cleanup #34

Merged
merged 6 commits into from
May 5, 2020
Merged

Plot cleanup #34

merged 6 commits into from
May 5, 2020

Conversation

breznak
Copy link
Member

@breznak breznak commented Apr 30, 2020

  • fix the issue with incorrect TP,FP in Plot
  • cleanup plot code

for Results visualization.
Fixes broken TP,FP computation.
major cleanup
into the single file
(it was a trivial plot, added it to the former "Results" notebook.)
to avoid the spaces in file name in Linux
@breznak breznak requested a review from Zbysekz April 30, 2020 14:05
@breznak
Copy link
Member Author

breznak commented Apr 30, 2020

@Zbysekz can I have your review on this PR? I've addressed the issue with TP,TN we've talked about. And some cleanup.
I've saved the files without figures, so the ipynb is much smaller. (and easier to diff later changes)

Copy link

@Zbysekz Zbysekz left a comment

Choose a reason for hiding this comment

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

Nice changes. Good thing about the easier diff.

  • In the second plot ( In[6] ) i can't see any data for "standard score", just the legend.
  • The third plot ( In[7] ) shows just input value and input value mean? I would add some header comment on the beggining of In[7] to separate that
  • Two first plots uses same data file that user can change at the beggining ("detector_summary_row") but third plot uses different data file?

@Zbysekz
Copy link

Zbysekz commented May 3, 2020

About the line in In[13] with comment "TODO is this branch needed?" i suggest to delete the condition and also the second branch with zero indexes index. Add this line instead: assert summaryDataFrame["Threshold"][detector_summary_row].size==1, "There is multiple records in summary file for one datafile!"
I can't figure out why we would need to have multiple records per one datafile...

@breznak
Copy link
Member Author

breznak commented May 4, 2020

Two first plots uses same data file that user can change at the beggining ("detector_summary_row") but third plot uses different data file?

yes, the main modifications were to "Plot Results ..." file, while there was also a "Plot Data " which did just that, a single plot of the data file. I've decided to merge it just in a single file (?)

About the line in In[13] with comment "TODO is this branch needed?" i suggest to delete the condition and also the second branch with zero indexes index.[...] I can't figure out why we would need to have multiple records per one datafile...

I guess we can do that. I'd think it's for a compatibility with some older format, but currently NAB shouldn't change that much. I'll rm that

fixed the value that wasn't plotted in recent changes.
But I'm not sure about the meaning/correctness of the data.
@breznak breznak requested a review from Zbysekz May 4, 2020 20:12
@breznak
Copy link
Member Author

breznak commented May 4, 2020

In the second plot ( In[6] ) i can't see any data for "standard score", just the legend.

thanks for spotting that. I've fixed the mistake I introduced and the data is being plotted.
To be honest, I'm not sure about the meaning of that data and which block of code creates that. It's the 'NAB manually specified data of how anomaly score should go', and it shows -0.11 for non-anomaly etc.
So we're again plotting that, but later we should look at how it's computed.

@breznak
Copy link
Member Author

breznak commented May 4, 2020

Thank you for your review, @Zbysekz . Please re-review when you have time, I've adressed the issues.

Copy link

@Zbysekz Zbysekz left a comment

Choose a reason for hiding this comment

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

Good to go

@breznak breznak merged commit 67eb6e5 into master May 5, 2020
@breznak breznak deleted the plot_cleanup branch May 5, 2020 05:21
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