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 template for multiple weights with diff between histogram with di… #354

Merged
merged 26 commits into from
Jun 1, 2024

Conversation

pl0xz0rz
Copy link
Contributor

This PR:
-Fixes a major bug with generated javascript for using columns from different histograms/dataframes in one expression
-Fixes a bug with counting the length in joins on the client
-Adds a prototype implementation of #352

@miranov25
Copy link
Owner

Thanks. One test failed:

=============================
FAILED test_ClientSideJoin.py::test_join - TypeError: unsupported operand type(s) for +: 'NoneType' and 'str'
========================================================================================================== 1 failed, 43 passed, 283 warnings in 86.91s (0:01:26)

@miranov25
Copy link
Owner

Template layout for tabs

We can make diff in respect to the varNorm - this is done per variable and we can compare different statistics with different weights

Combinations:

  • normVar - [Yes,No], normWeight - [Yes,No]
  • 3 value - 2D,3D, 4D

Old templated had only normVar - [Yes, No]

Test layout tabs

  • histXY. - as before
  • histXYWeight
    0,0 - mean, median weight
    0,1 - rms for weght
    0,2 -entries for wight
    1,0 - mean, median for reference wight
    1,1. - -entries for reference wight
    1,2 - -entries for reference wight
    2,0 - diff mean, median weight. (diff - minus or ratio)
    2,1 - diff rms for weight (diff - minus or ratio)
    2,2 -diff entries for wight (diff - minus or ratio)

@miranov25
Copy link
Owner

#Layout to test histoXYZWeight
similar as above but with colorZ
histXYWeight
0,0 - mean weight
0,1 - rms for weght
0,2 -entries for wight
1,0 - mean, median for reference wight
1,1. - -entries for reference wight
1,2 - -entries for reference wight
2,0 - diff mean, median weight. (diff - minus or ratio)
2,1 - diff rms for weight (diff - minus or ratio)
2,2 -diff entries for wight (diff - minus or ratio)

@miranov25
Copy link
Owner

Thanks for the commit! The unit tests passed successfully.

Here are some comments on the implementation based on the screenshot:

  • Data Proportions: The proportions for the data should remain unchanged. However, the height for the "Weight" version should be increased by approximately 1.15 (or 15%).
  • histoXYweight Entries Column: I believe it would be more appropriate to use a 2D representation for the entries column, specifically "entries:X". Currently, it's displayed in 3D as "entries vs Y vs color(X)". My preference is to maintain the same X-axis for both visualizations.
  • Diff Plot X-Axis Title: If possible, please ensure the X-axis title in the diff plot matches the title used above.
image

@miranov25
Copy link
Owner

All tests are OK

There are many warnings - related to the unit test

  • 44 passed, 403 warnings in 71.65s (0:01:11)

After adding pytest.ini with unit test the amount was reduced to the 53

  • 44 passed, 53 warnings in 61.38s (0:01:01)

I will commit the pytest.ini after merging

(venv7) Singularity> cat $RootInteractive/pytest.ini
[pytest]
markers =
  unittest: description for the unittest marker

@miranov25
Copy link
Owner

I briefly reviewed the code and did not identify any obvious issues.

We will need to develop a new unit test in which we prepare specialized data and define invariants to facilitate easier testing of self-consistency.

I am merging the pull request, but the issue remains open for now.

@miranov25 miranov25 merged commit dc8bf5c into miranov25:master Jun 1, 2024
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