-
Notifications
You must be signed in to change notification settings - Fork 35
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
feat: hist_juxtaposition #245
Conversation
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.
Nice work so far. The reports notebook needs to be updated so that the tests pass. After, it's easier to review the resulting report.
Now that the issue in histogrammar should be resolved, we can test if that results in a passing ci/cd here. @pradyot-09 Until there is a new histogrammar release, you could add the git branch to
|
@pradyot-09 |
The current implementation is a great step forward, as it allows users to view individual histograms, which wasn't possible before: The implementation might be better using two plotly dropdowns in the same figure. This removes the redundant information that is now present in the plots: the reference is available both left and right, and histogram_prev1 is the same as histogram once selected. |
1174c6d
to
3249fac
Compare
@sbrugman Just to clarify: A single plot with a dropdown to change the dates? asking because we discussed one of the main reason to have the plots side by side was to compare them. |
Side-by-side or overlay should be both fine. The confusion in the current approach is the redundant information (histogram_prev1 may be the same as histogram in the other). You could consider adding the reference to the drop-down, and showing one histogram per drop-down. |
3249fac
to
f429feb
Compare
@sbrugman Have pushed the histogram plots with updatemenus. The code looks neater now. For static references the "histogram_ref" plots are repetitive. I think it would be clever to have the "histogram_ref" only once for static references. However, I think that we would need to pass the Settings object rather than the Report object to report_generation(). Not sure about the best way to get reference_type in histogram_section. |
@sbrugman pushed the new histogram inspector with overlays. For now, I have kept double dropdowns for static reference too because I thought it was just very easy to disable overlays if needed. Let me know what you think. |
a6e383c
to
deedc49
Compare
deedc49
to
69dfc6e
Compare
Co-authored-by: Pradyot Patil <pradyotpatil@gmail.com>
See #230 Co-authored-by: Pradyot Patil <pradyotpatil@gmail.com>
69dfc6e
to
6488f3d
Compare
For now, the last_n is by default set to 2. Therefore, only two dates would appear in the dropdown. For the airline dataset if the last_n is set to max, popmon runs into the issue (for DEPARTURE feature) raised by Tomek #244.
closes #230