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

Change semantic equivalence logging #304

Closed
wants to merge 8 commits into from
Closed

Change semantic equivalence logging #304

wants to merge 8 commits into from

Conversation

khdesai
Copy link
Contributor

@khdesai khdesai commented Nov 19, 2019

Generalize semantic equivalence-contributing properties checking functionality within environment.py and add prop_scores dict so all scoring results can be in a single python object.

Current test suite includes tests that do and do not specify a comparison method, so did not add tests at this time. But willing to do so if needed

@codecov-io
Copy link

codecov-io commented Nov 19, 2019

Codecov Report

Merging #304 into stix2.1 will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           stix2.1     #304      +/-   ##
===========================================
- Coverage    98.15%   98.13%   -0.02%     
===========================================
  Files          123      123              
  Lines        13845    13730     -115     
===========================================
- Hits         13589    13474     -115     
  Misses         256      256
Impacted Files Coverage Δ
stix2/environment.py 100% <100%> (ø) ⬆️
stix2/test/v21/test_environment.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4c0115...318bbf6. Read the comment docs.

@clenk clenk changed the title Change logging Change semantic equivalence logging Nov 20, 2019
Copy link
Contributor

@clenk clenk left a comment

Choose a reason for hiding this comment

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

Looks good! As we talked about, just a few things to change:

  • Test weights not adding up to 100
  • Remove _indicator_checks and fix the 2 tests that use it
  • Make sure we have a test for if "method" is provided
  • Reshuffle the try/excepts in semantically_equivalent() and adjust warning messages
  • Add param to semantically_equivalent() for prop_scores dict
  • Update documentation on using the weights dictionary (both with tuples or 'method'), and how to get the detailed results

The documentation piece may go in a separate PR depending on timing.

@clenk clenk closed this in 74eeaba Dec 23, 2019
@clenk
Copy link
Contributor

clenk commented Dec 23, 2019

Thanks, @khdesai! These changes have been merged in.

@emmanvg emmanvg added this to the 1.3.0 milestone Jan 15, 2020
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.

4 participants